Application of suggestion #633

[SantiagoBautista]

This Pull request offers a solution for issue #633 .

If annotations take argumentList instead
of either expressionList or keyValueList,
the exact same behavior is acheivable,
but without introducing ambiguity into the grammar.

[mihaibudiu]

The compiler implements this and bison did not complain about an ambiguity.

[ChrisDodd]

The grammar in the compiler does not allow an empty keyValueList, so there's no ambiguity. Why the grammar in the spec was different, I don't know.

[mihaibudiu]

Annotations never had a clear semantics in the spec, but they have an internal compiler api to access them, which will need to change if we take this pr.

[mihaibudiu]

Although this does simplify the grammar, it changes the set of programs accepted (as you point out) and possibly it may require a different representation for the Annotation IR class in the compiler. I believe that this fix makes things more complicated. The right fix is to not accept an empty key-value list.

[SantiagoBautista]

The compiler and the spec already differ in many ways:

• In the compiler there is a distinction between realTypeArgumentList and typeArgumentList that is not present in the grammar
• In the compiler abstract methods and abstract method instantiations are supported for externs, and it is not the case in the grammar
• The compiler supports value_set declarations in parsers and they are not present in the grammar
  etc

If this pr simplifies the grammar, is it really a problem if it temporarily adds another difference between the compiler and the grammar?

[hanw]

The abstract method is an experimental feature and has not been accepted to the spec yet.

[mihaibudiu]

1. The bison grammar should have clear comments on all features that are still experimental. If not, that's a bug.

2. I personally prefer to have first an implementation and then a spec change - because this way we are sure that the proposal works. Sometimes when we do it the other way round we discover that we proposed things that are very hard or impossible to implement, and then we are in a really bad position.

3. We try to keep the bison grammar and the spec in sync, but it's not easy, since they cannot be generated automatically, especially the fragments in the spec. If you find a difference you should file a bug.

4. Value sets will appear in the next version of the spec - the draft in the repo should have that text already.

5. The problem is that the change you proposed changes the set of programs that are accepted. Your grammar will accept annotations that have both key-value pairs and lists of expressions. Today the implementation accepts one of the two, but not both. Implementing this will require some work - the classes we have today in the IR are not sufficient if you want to preserve the order of annotations on an object.

This also changes the (not explicitly specified) contract between the front-end and the back-end: today we assume that a back-end can check for annotations in one of the two formats, but with your change back-ends have to be ready to accept annotations that have a mix of formats.

6. In general, we should try to optimize for the language users, not for the language implementers. I will prefer a more complicated grammar if it makes the language easier to use, describe and understand.

[SantiagoBautista]

I agree with @mbudiu-vmw that not accepting the empty keyValueList is a solution to ambiguity that does not require changing the compiler in any way (because it actually correspond to putting the compiler's current behavior into the grammar).

Since the proposition in this PR seems to need further discussion, I will do a PR implementing @mbudiu-vmw proposition, that shouldn't raise any problem, and we will see for this PR later. What do you think?

PS : I hadn't seen your latest comment when I wrote this one

[mihaibudiu]

The problem is that we will release a new specification this week, so whatever choice we make will become official. If this needs further discussion we won't be able to release this part of the spec.
Perhaps there is an escape, since your grammar accept a strict superset of the programs accepted by the simpler proposal, we can make such a change later.

[SantiagoBautista]

Ok. So I suggest that for the spec release we take your proposition: requiring in the grammar that the keyValueList s are non-empty.
And I do think that doing so (changing the grammar, in one way or another) is necessary, because the current grammar is ambiguous about that point (and, in addition, does not currently match the implementation, that does require the keyValueList to be non-empty).

[SantiagoBautista]

I will also make sure to open a pull request that updates the grammar concerning value_set in parsers, because there are currently some rules missing in the grammar (and present in the implementation, so it is a bug in the grammar).

[mihaibudiu]

I appreciate.
If they are not marked experimental it's a bug, either in the grammar or in the spec.

[jnfoster]

Closing this for now...