Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented May 14, 2024

…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.

…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.
@davidmorgan
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

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.

Copy link
Member Author

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!
Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

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,
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

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,
Copy link
Contributor

@jakemac53 jakemac53 May 30, 2024

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.

Copy link
Member Author

@lrhn lrhn May 31, 2024

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".)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants