-
Notifications
You must be signed in to change notification settings - Fork 198
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
Split augmentations into improved part files and augmentation declara… #3800
base: main
Are you sure you want to change the base?
Conversation
…tions. Unifies "augmentation libraries" and part files. Part files can have imports, exports and further part files. Part files can use configurable URIs. A `part of` directive can no longer use a library name. Part files inherit the imports of their parent file, and can extend or shadow those with their own imports. Augmentation declarations can occur in any part or library file. An augmentation must occur "below" the declaration it augments: Either later in the same file ("below" when viewing code), or in the file-tree of a part file of the same file ("below" in the part-file tree of the library). This ensures that the original declaration occurs above all augmentations of it, and that all augmentations of the same original declaration occur on a single path down the part-file tree of the library. What again ensures that reordering `part` directives does not change the order of augmentations. Changed the lexical scope of augmenting class-like declarations (declarations with a member scope) to only contain the members declared inside the same class-like declaration, not the collection of all members declared by all declarations with the same name. Rewrote the part about applying augmentations. This is more speculative since it doesn't provide a complete definition, but more of a pattern for extending semantics that assume a name refers to a single declaration, into one where a name denotes a set (stack depending on augmentation application order), of individual syntactic declarations. The existing semi-syntactic "merging" may not be a viable specification approach, since it requires merging code from different scopes into a single scope. That's not impossible, we also move code around for mixin applications, but it's also easy to get wrong.
Whoops, I thought this was an issue; it's a PR. Opened #3848 so we have an issue for this. |
macros and then produces one or more new part files that contain all | ||
of the changes that the macros made to the library where they are applied, as | ||
new declarations to be added or augmentations that modify existing | ||
declarations. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary newline
Authors: rnystrom@google.com, jakemac@google.com, lrn@google.com <br> | ||
Version: 1.0 (See [Changelog](#Changelog) at end) | ||
|
||
This is a stand-along definition of _improved part files_, where the title of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a stand-along definition of _improved part files_, where the title of | |
This is a stand-alone definition of _improved part files_, where the title of |
Pre-feature part files inherit the entire import scope from the library file. | ||
Each declaration of the library file and each part file is included in the | ||
library’s declaration scope. It’s viable to think of part files as being | ||
textually included in the library file. There is a is even a rule against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
textually included in the library file. There is a is even a rule against | |
textually included in the library file. There is even a rule against |
itself, and its transitive part files. A library is defined by the source code | ||
of its library file and *all* transitively included part files, which can be an | ||
arbitrarily deep *tree*. A part file inherits the imports and import prefixes | ||
of its parent file (the library or part file that included it) into its |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets clarify if this is transitive or not (presumably it is). In other words, you inherit the entire "import scope" of your parent, which includes that parents parents import scope, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is transitive.
The import scope of a part file extends the import scope of its parent file (which then transtively extends its parent's import scope).
Let's try to find an understandable way to say that.
declarations in the library file and all transitive part files are equal, | ||
and are all in scope in every file. They introduce declarations into the | ||
library’s declaration scope, which is the most significant scope in all | ||
files of the library. If there is any conflict, top-level declarations win! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually not sure this is the correct choice.
Consider that declarations which are not in the current file are not in the lexical scope. We could thus consider them to be equivalent in terms of priority to other declarations in the import scope. In terms of the intuition provided by lexical scoping, I think this makes sense, both declarations are coming from a separate file, and in the case of conflicts it might be best to require a prefixed import or using show/hide to disambiguate.
This would implicitly make shadowing a top level member from an import via an augmentation become an error (it no longer shadows, it becomes a conflict), which might also help to resolve some of the trickier macro questions around shadowing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually not sure this is the correct choice.
Consider that declarations which are not in the current file are not in the lexical scope
Thta's a crazy idea. I just might work!
They are in scope today, but today there is only one scope in a library. It's the same scope that is the top-level scope for every file of a library.
What I changed here is to have the import scope be per-file, but then I applied the same declaration scope on top of every import scope. There is no change to that.
If we also inherit the declaration scope, and can extend on top of that, ... how would that work?
- A part file's top-level scope extends the declaration scope of its parent, not its import scope.
- It extends with its own import scope and prefix scope (as currently specified) and then with its own declarations and the declarations of all its sub-parts, but not the declarations of every file in the library.
- A library file does need to have access to the declarations of its parts, so it needs to include every declaration in the library in the library file's declaration scope. Anything else would be too breaking.
- A part file inherits its parent's top-level scope (imports,prefixes,all declarations in all subparts, including this one), but can override anything in there with an import, which can then be overridden by the declarations contained in the part file and all its sub-parts.
- If the part file needs to see any declaration from the parent file, which are also the declarations of its sibling parts and any non-shadowed declarations from the rest of the library, it should avoid shadowing with an import or prefix.
(Hmm. Feels familiar. I'm pretty sure I've tried to specify something like that before. Probably in one of the earlier augmentation issues, or maybe in a document I never published.)
That all seems like it could work.
And it even has some nice properties. It would mean that any declaration that only exists in a parent or sibling part can be ignored.
A declaration in a sibling will no longer conflict with a prefix in this part.
Depending on how augmenting declarations count, a name may be declared in more than one place.
So about "shadowing a top level member from an import with an augmentation", that would mean declaring an augmentation with a name which would otherwise resolve to an imported declaration from another library. That doesn't completely make sense if the augmentation itself introduces the name. But maybe it should only ever be the base declaration which introduces the name.
So not sure that situation needs to be an error, but it can be.
- One can see an augmenting declaration as referencing the declaration it augments, and therefore the chosen name should resolve to that declaration. If it resolves to something that cannot be augmented, that's just a plain error.
- Or one can see the augmenting declaration as simply having the same name as the declaration it augments (that's how it's decided what it augments). We don't resolve the declared name of a declaration. It introduces a name, it doesn't reference one.
In the latter case, there is no need for the augmented declaration to be in scope. The only question is whether the augmenting declaration introduces the name or not. (But if not, that may actually suggest that the former interpretation is the right one.)
The former case, based on that thought, could mean that augmenting declarations do not introduce a name in the surrounding scope (which now matters because the declaration scope is not every declaration in the library any more), and the name of the declaration is a reference to the declaration it augments. You can only refer to declaration's name if you can refer to the base declaration, and you can only augment it if you can refer to it.
For augmenting member declarations, the name should be resolved relative to the type that is being augmented. A static member or constructor should be resolved against the static scope of the class/mixin/etc that it's inside (including all earlier augmentations, even inside the same scope). An instance member is resolved against the interface of the surrounding type (or whatever makes sense for extensions) also including all earlier augmentations.
So, op problem, those are not lexical scope based. If we can refer to the container declaration, we can augment its members
So, TL;DR:
- The top-level scope of the library file is the same one as today (imports, prefixes, all declarations in the library).
- The top-level scope of a part file extends the top-level scope of its parent file with its own imports, prefixes and all non-augmenting declarations in its sub-tree.
- Augmenting top-level declarations do not introduce a name in their surrounding scope, instead the name in the augmenting declaration refers to the declaration they want to augment, which must be a declaration of the current library, and must be in scope. (So if they work, they don't need to introduce a name, if they don't, they'd be referring to themselves, which makes no sense.)
- Augmenting namespaced declarations (instance, static, constructor) may or may not introduce a name in the lexical scope. It doesn't hurt, and it allows an augmenting declaration body to safely be recursive, without risking a conflict with an imported name.
SGTM. Let's ask others if they can see any issues.
|
||
*Since repeating the type parameters is, by definition, redundant, this | ||
doesn't accomplish anything semantically. But it ensures that anyone reading | ||
restriction doesn't accomplish anything semantically. It ensures that | ||
anyone reading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reflow text
defined solely by the original function.* | ||
|
||
* An augmenting declaration uses `augmented` when the original declaration has | ||
no concrete implementation. Note that all external declarations are assumed | ||
to have an implementation provided by another external source, and they will | ||
throw a runtime exception when called if not. | ||
|
||
**TODO: Should we allow augmenting functions to add parameters? If so, how does | ||
this interact with type checking calls to the function?** | ||
**TODO: Should we allow augmenting functions to add optional parameters? If so, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove this, we have pretty well decided this will not be allowed
**TODO: Should we allow augmenting functions to add optional parameters? If so, | ||
how does this interact with type checking calls to the function?** | ||
|
||
**TODO: Should we allow an augmenting function to repeat default values? Or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not allowed and can be removed
how does this interact with type checking calls to the function?** | ||
|
||
**TODO: Should we allow an augmenting function to repeat default values? Or | ||
change default values? Invoking the `augmented` function should will supply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change default values? Invoking the `augmented` function should will supply the | |
change default values? Invoking the `augmented` function will supply the |
* The original constructor is a non-redirecting factory constructor and the | ||
augmenting constructor has an initializer list. | ||
|
||
* The augmenting constructor does not need to repeat the `factory`, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would require factory
to be repeated, I think it makes it easier to understand the constructor when looking at the augmentation.
Consider for instance jump to definition takes you right to the augmentation, it would be good to know this is a factory you are looking at.
* If a library file has a language version marker, | ||
then it’s a compile-time error for any sub-part file | ||
to not have the same language version marker. | ||
* If a library file has no language version marker, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok hear me out - what if you have a part which is defined in a different package, with a different default language version. Then you are required to have a language marker in those parts.
Should we explicitly block parts from other packages? Generally the spec doesn't talk about packages, but it seems like a reasonable constraint.
If we do allow it, then I think this should be specified not in terms of the existence of a language version marker, but in terms of the actual computed language version, which must be the same across all parts and the root library file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok hear me out - what if you have a part which is defined in a different package, with a different default language version. Then you are required to have a language marker in those parts.
And that's the reason I explicitly specified that as an error. 😎
Should we explicitly block parts from other packages?
We should, and I did! (Line 438, just below, foreshadowed in line 413 above).
Part files must belong to the same package as their parent file. That ensures they have the same default language version.
We always should have said that, precisely because of this issue.
We've said that a library file and its part file must have the same language file marker, or none.
What we really mean is that they must have the same language version, but we also want that to stay true if someone changes the SDK version.
The best way™ to ensure that they have the same version no matter what else changes is to require either all having the same language version marker override, or all having the same default version, and the latter is implied by being in the same package.
(Although I can see I technically only have the impliciation in one direction, if the library file is in a package, so must its part files be, but that should also be "and vice versa".)
…tions.
Unifies "augmentation libraries" and part files.
Part files can have imports, exports and further part files. Part files can use configurable URIs.
A
part of
directive can no longer use a library name. Part files inherit the imports of their parent file, and can extend or shadow those with their own imports.Augmentation declarations can occur in any part or library file. An augmentation must occur "below" the declaration it augments: Either later in the same file ("below" when viewing code), or in the file-tree of a part file of the same file ("below" in the part-file tree of the library).
This ensures that the original declaration occurs above all augmentations of it, and that all augmentations of the same original declaration occur on a single path down the part-file tree of the library. What again ensures that reordering
part
directives does not change the order of augmentations.Changed the lexical scope of augmenting class-like declarations (declarations with a member scope) to only contain the members declared inside the same class-like declaration, not the collection of all members declared by all declarations with the same name.
Rewrote the part about applying augmentations. This is more speculative since it doesn't provide a complete definition, but more of a pattern for extending semantics that assume a name refers to a single declaration, into one where a name denotes a set (stack depending on augmentation application order), of individual syntactic declarations.
The existing semi-syntactic "merging" may not be a viable specification approach, since it requires merging code from different scopes into a single scope. That's not impossible, we also move code around for mixin applications, but it's also easy to get wrong.