Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix container types calculation #1055

Merged
merged 2 commits into from
May 25, 2023
Merged

Fix container types calculation #1055

merged 2 commits into from
May 25, 2023

Conversation

pluralia
Copy link
Contributor

Closes #150.

  • drops union types from the calculation of container types
  • drops sharing of container types over connected components: now the container types are lifted only lifted to the parents
  • if one of the children has no container types, the parent also loses container types

Examples:

interface A extends AstNode { $container: E | F | G; strA: string }
interface B extends AstNode { $container: E | F | H; strB: string }
interface C extends A, B { $container: E; strC: string }
interface D extends A, B { $container: F; strC: string }
interface E extends AstNode{ c: C }
interface F extends AstNode{ d: D }
interface G extends AstNode{ a: A }
interface H extends AstNode{ b: B }

X has no container types -- A and B, too

interface A extends AstNode { strA: string }
interface B extends AstNode { strB: string }
interface C extends A, B { $container: E; strC: string }
interface D extends A, B { $container: F; strC: string }
interface X extends A, B { strC: string }
interface E extends AstNode{ c: C }
interface F extends AstNode{ d: D }
interface G extends AstNode{ a: A }
interface H extends AstNode{ b: B }

@pluralia pluralia requested a review from spoenemann May 21, 2023 12:01
@spoenemann spoenemann added this to the v2.0.0 milestone May 23, 2023
Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks mostly good (found only one wrongly computed container in the grammar).

@@ -418,7 +411,7 @@ export function isGroup(item: unknown): item is Group {
}

export interface Keyword extends AbstractElement {
readonly $container: Alternatives | Assignment | CharacterRange | CrossReference | Grammar | Group | NegatedToken | ParserRule | TerminalAlternatives | TerminalGroup | TerminalRule | UnorderedGroup | UntilToken;
readonly $container: CharacterRange;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong: CharacterRange is not the only possible container of Keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spoenemann, it's correctly for the current calculation:

  • Keyword is used in the assignment in the only one place -- in CharacterRange
  • One more usage is the rule PredicatedKeyword that infers Keyword -- 1) the interface PredicatedKeyword will not be generated by the grammar definition, so no its income to container types; 2) if we would have the interface PredicatedKeyword, it would be a child of Keyword -- no income to container type, too, because we don't lift children container types in parents anymore
  • Other usages are in type alternatives and are ignored by the new algorithm: AssignableTerminal, CrossRefTerminal, AbstractTerminal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a rather complex chain of unassigned rule calls here:

Alternatives > ConditionalBranch > UnorderedGroup > Group > AbstractToken > AbstractTokenWithCardinality > AbstractTerminal > Keyword

Alternatives has two possible containers: Group and ParserRule. Keyword should inherit those containers.

I've seen that in simpler examples of unassigned rule calls, the container types are fine. So maybe this is just a special case for which we can create an issue, but we ignore it for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By my understanding it's not an issue, but an expected behaviour for the current algorithm.

The moment is the algorithm is working with inferred / declared types (from the ast.ts, basically) and not with parser rules content. That means, not all information that you can see in the .langium is available for the algorithm. As a result:

  1. Most of types with the names from the chain doesn't exist
  2. Group and ParserRule contain only AbstractElement and not Alternatives

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let's ignore the issue for now and get back to it in case it turns out to be a real problem. I created #1063 so we don't forget.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pluralia pluralia merged commit 3fadd71 into main May 25, 2023
3 checks passed
@pluralia pluralia deleted the pluralia/container-prop branch May 25, 2023 14:16
@spoenemann spoenemann modified the milestones: v1.3.0, v2.0.0 Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve container types calculation
2 participants