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

Consistency of type usage #429

Closed
spoenemann opened this issue Feb 28, 2022 · 6 comments · Fixed by #436 or #548
Closed

Consistency of type usage #429

spoenemann opened this issue Feb 28, 2022 · 6 comments · Fixed by #436 or #548
Assignees
Labels
grammar Grammar language related issue types Types related issue validation Validation related issue
Milestone

Comments

@spoenemann
Copy link
Contributor

See #406 (comment): we should validate usages of types for consistency. For example, this is not valid because Foo is not an AST node type.

type Foo = string;

Bar returns Foo: ...

The same applies to actions and extended interfaces.

@spoenemann spoenemann added the grammar Grammar language related issue label Feb 28, 2022
@spoenemann spoenemann added this to the v0.3.0 milestone Feb 28, 2022
@spoenemann spoenemann added the validation Validation related issue label Feb 28, 2022
@msujew msujew self-assigned this Mar 2, 2022
@spoenemann spoenemann added the types Types related issue label Mar 4, 2022
@pluralia
Copy link
Contributor

We validate usages of types from declared types, we should validate also types from inferred types.
For example, Statement infers the type type Statement = Definition | Evaluation and not an interface.

Statement:
	Definition | Evaluation;

So, interface Definition should be not able to extend Statement like here:

interface Definition extends Statement {
    name: string
    expr: Expression
    args: DeclaredParameter[]
}

@pluralia pluralia reopened this Mar 18, 2022
@pluralia pluralia modified the milestones: v0.3.0, v0.4.0 Mar 18, 2022
@montymxb
Copy link
Contributor

@msujew I see you're still assigned to this after it was closed and reopened. Are you still interested in working on this one, or is it okay if I go for it?

@msujew
Copy link
Member

msujew commented May 30, 2022

@montymxb Feel free to take this one 👍

@msujew msujew assigned montymxb and unassigned msujew May 30, 2022
@montymxb
Copy link
Contributor

montymxb commented Jun 8, 2022

@pluralia @msujew while working through this problem I had an idea I wanted to run by you both. Would it be considered reasonable to prevent interfaces from extending inferred types? And by extension, just saying that declared types cannot extend inferred types.

I have some thoughts why this might be helpful:

  • it clearly separates the fact that declared types exist on a separate level from parser rules
  • we don't have to try to report errors that stem from inferred types in this case
  • it removes having to compute inferred types (and unions) to validate existing type hierarchies
  • it supports that declared types are unambiguous in their structure and inheritance

In this way, you can make something explicit, but you cannot set it back by basing it on something inferred.

I am trying to think if this restricts anything in terms of expressiveness, but I haven't come up with anything yet 🤔 .

@spoenemann spoenemann modified the milestones: v0.4.0, v0.5.0 Jun 9, 2022
@spoenemann
Copy link
Contributor Author

I agree with that, extending an inferred type with a declared type is weird and should not be supported.

@pluralia
Copy link
Contributor

I like the idea, here is some thoughts.

it clearly separates the fact that declared types exist on a separate level from parser rules

We are going to implement the strict mode specially for the case.

it removes having to compute inferred types (and unions) to validate existing type hierarchies

We still have to do it for validation purposes.

One of the most important advantages of Langium is the brief prototyping: you can write a couple of parser rules and everything works out of the box. Sometimes you need to add declared types in addition to this pair of parser rules for minimal type shaping, in which case it's no good to prohibit extending inferred types, because you would have to add more declared types, which goes against the idea of brief prototyping "as few entities in grammar as possible".

Another point is that inferred and declared types will become semantically "unequal": declared types can be extended, while inferred types cannot.

To summarize: I think we should do it in the strict mode only and allow it by default for brief prototyping.

We shortly discussed this problem, and @montymxb suggested using warnings when inferred types are extended by declared types. I fully support this idea: we keep prototyping brief, but at the same time notify the user when a declared type is "unstable" and can be implicitly changed because of the parent inferred type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grammar Grammar language related issue types Types related issue validation Validation related issue
Projects
None yet
4 participants