Type Usage Consistency (2) #548
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lotes
reviewed
Jun 21, 2022
|
|
||
| test('Interfaces should only be able to extend inferred interfaces', async () => { | ||
| const validationResult = await validate(` | ||
| grammar G |
There was a problem hiding this comment.
I would vote for speaking names, it is hard to read
|
@Lotes updated that test message you pointed out, should be clearer now. |
msujew
reviewed
Jun 30, 2022
packages/langium/test/grammar/langium-grammar-validator.test.ts
Outdated
Show resolved
Hide resolved
Lotes
reviewed
Jul 1, 2022
|
|
||
| // just for the sake of generating a pair of inferred interfaces | ||
| InferredI1: 'InferredI1' InferredI1=ID; | ||
| InferredI2: 'InferredI2' InferredI2=ID; |
There was a problem hiding this comment.
Confusing… rule name equal to assignment property?
Lotes
reviewed
Jul 1, 2022
Lotes
reviewed
Jul 1, 2022
| // we can extend an inferred type that has no parent | ||
| interface DeclaredExtendsInferred1 extends InferredI2 {} | ||
|
|
||
| // we can extend an inferred type that extends other inferred types |
There was a problem hiding this comment.
Comments could be the name of the new test.
There was a problem hiding this comment.
Comments have to explain, why something is what it is. I think we should form a code so that it is easy to read without checking comments… Comments can outdate, very fast.
Lotes
reviewed
Jul 1, 2022
|
Bringing this in w/ the prior approval, just made the tests a little more unit-esque. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #429 (again) by extending the same super type checks done for declared types to inferred super types, extending upon the existing checks that were first applied for this issue. These checks include a warning against extending declared types by inferred types. For more details on why this is an issue, see the discussion in the original issue, which has some good conversation on the issue.
This was done by tapping into the inferred type collection already in the type system, but is only applied to a singular ParserRule. This gives enough information to be able to determine whether a type union would be ultimately generated, and in such a case we can give a validation error. In the case where no type union is generated, we can assume that an interface would be generated, and can instead apply a warning.
Hopefully this achieves the original goals without introducing too much overhead by pulling in some of the type validation logic. Ideally, it would be nice to keep those two steps more separate, but this seemed better than trying to duplicate some of the raw AST type inference logic in the grammar validation phase.