eclipse-langium / langium

Next-gen language engineering / DSL framework
https://langium.org/
MIT License
754 stars 68 forks source link

Actions can rewrite declared types #514

Closed montymxb closed 2 years ago

montymxb commented 2 years ago

Actions can rewrite declared types, effectively erasing/changing their previously declared properties. This leads to an inability to reason about declared types as a fixed source of information about the resulting AST that is generated.

An example grammar in Langium that causes this:

// 1. define some interface
interface A {
    val: string
}

// X inherits correctly from A via a code action, leaving A unchanged
X: 'x' {A} val=ID;

// but Y redefines A entirely
Y: 'y' {infer A} q='broken';

assumes the ID terminal is declared

This generates the following interface, which is not correct:

export interface A extends AstNode {
    q: 'broken'
}

Langium version: 0.3.0 (dev)

Instead:

spoenemann commented 2 years ago

(Note: Code actions are an LSP request type, which is totally unrelated to actions in the grammar)

spoenemann commented 2 years ago

Not the same but related: #447

dhuebner commented 2 years ago

@montymxb

A is a declared type and cannot be inferred

I would like to propose: A is a declared type and cannot be redefined

montymxb commented 2 years ago

As an additional note, I've noticed that in the following code there's a retrieval of distinct interfaces and unions (both inferred & declared) after they have been combined together. What strikes me as odd is that there's room for duplication, and that in such a case inferred types take precedence over declared due to the order of concatenation.

https://github.com/langium/langium/blob/521e56692772cd1e9230694b66efe1e7fbbbe28f/packages/langium/src/grammar/type-system/type-collector.ts#L24-L33

I'm not proposing that we necessary do something about this in this issue, since validation can take care of it before this point. But I am curious if we should be doing something other than simple concatenation of two different groups of interfaces & types. Even if we do proper validation before-hand, if something sneaks through this could pop up again in the same fashion.

pluralia commented 2 years ago

But I am curious if we should be doing something other than simple concatenation of two different groups of interfaces & types.

As I understand it, the only thing we can do to be sure there are no errors at all is to repeat the validation. Since the validation is already done for the moment, I think we can leave this code as it is, at least until there is no other reason to improve it.