Simplify augmentations.#4357
Conversation
- Remove references to macros. - Don't allow augmentations to wrap or replace code. Remove support for `augmented` expressions. Disallow an augmentation from providing a body to a declaration that already has one. - Remove support for augmenting variables. - Simplify constructor augmentations: no concatenating initializers or merging initializers from one augmentation and a body from another. - Remove support for augmenting typedefs. - Remove support for augmenting redirecting constructors. - Allow a function augmentation to have an `external` body. There are still more changes I'd like to make. In particular, the grammar for members seems pretty hairy and I suspect could be refactored to be simpler, but I'm not sure if that will cause problems for other people who rely on the existing grammar rules.
lrhn
left a comment
lrhn
left a comment
There was a problem hiding this comment.
More shorter, more better!
I have a gazillion nitpicks and some questions (which might suggest that something could be said to answer that question, or which might suggest that I only read the changed parts and therefore lacked some context.).
And some comments that are better left for later.
Good clean-up!
|
cc @johnniwinther @scheglov in case you have thoughts on how this would affect the implementation. |
eernstg
left a comment
eernstg
left a comment
There was a problem hiding this comment.
LGTM, this is great! In particular, I appreciate the amount of regularization and simplification which is achieved by omitting some well-chosen elements.
Added a bunch of comments, of course. In particular, I think we should split out the discussions about the scoping (and possibly handle them during updates of the 'Part files with imports' feature spec).
|
|
||
| ## Syntax | ||
|
|
||
| The grammar changes are fairly simple. The grammar is modified to allow an |
There was a problem hiding this comment.
It is true that the spec parser contains all language features (such that it can be used to find syntax errors in tests involving features during their implementation). This ensures that it has been tested with new code which has been considered by authors/reviewers of tests, and also by implementation teams.
In other words, the spec parser is a rather practical tool when we need to ensure alignment with the current implementation.
There is no explicit marking of parts of Dart.g that are part of specific language versioned updates. The commit log in the topmost comment in Dart.g does have a reference to a language feature when it is introduced, but some language features have given rise to multiple updates (because the feature evolved in the meantime).
I think it makes sense to use Dart.g (this seems to be the case already), and thus include some features (like class modifiers), even though this doesn't map directly back to the grammar in the currently committed version of dartLangSpec.tex.
Another possibility could be to switch to PR #2605 (the proposed null safety update of the language specification) and run tools/plaintext_grammar.dart on that version of dartLangSpec.tex. However, this introduces yet another level of potential change before that PR is landed, and it doesn't have the ongoing sanity check relative to the implementation.
| a single name, a single type or function signature, and it's what all | ||
| references to the *name* refers to inside and outside of the library. | ||
|
|
||
| Unlike before, that single *semantic declaration* now consists of multiple |
There was a problem hiding this comment.
I can't comment on line 896, but I think it would be a good idea to replace non-augmenting by introductory, for consistency.
There was a problem hiding this comment.
Line 928 ends in "the C doe". That sentence needs to be revised and completed. Also, the occurrence of abstract (in code font) in a description of an "abstract method" is confusing: The method is abstract, but it can't contain the keyword abstract.
There was a problem hiding this comment.
I'll be honest, I'm not even really sure what this "Dynamic Semantics" section is all about. As far as I'm concerned, all we need to do is specified how augmentations are applied and then the resulting declaration's dynamic semantics are exactly as if you had written everything in one declaration (modulo scoping across part files, of course).
This looks like @lrhn's writing style. (@kallentu and I were coincidentally discussing today how we can tell who wrote each part of a spec 😄 ). @lrhn, do you know what this is supposed to be completed with? Do we need this section at all?
|
I'm going to go ahead and land this, but I plan to keep making tweaks, so feel free to comment here or file issues if any of you see other things. Thanks for the excellent reviews! |
6 participants
augmentedexpressions. Disallow an augmentation from providing a body to a declaration that already has one.externalbody.There are still more changes I'd like to make. In particular, the grammar for members seems pretty hairy and I suspect could be refactored to be simpler, but I'm not sure if that will cause problems for other people who rely on the existing grammar rules.