-
Notifications
You must be signed in to change notification settings - Fork 214
Inheritance and shadowing between parent files and part files. #3862
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
Comments
I think it is useful to consider the scopes of the parent files as well as the parts, to develop a consistent and principled approach here. In particular, there are actually three declaration scopes to consider - the current files declaration scope, the parent (part of) declarations scope, and the declaration scopes of all the parts (children) of the current file. I am quite opposed to the idea of any shadowing of anything from any of these declaration scopes. That seems highly unexpected - and we should either have the declaration scopes shadow all the import scopes, or produce a conflict. What I think would be a simple mental model here, is to make most things be conflicts. The only shadowing that would happen is the current files declaration scope would be allowed to shadow the import scope (both inherited and current). No other shadowing at all would happen, everything else is a conflict.
Essentially, stuff in the pure lexical scope can shadow stuff in the import scope, and nothing else can. |
Each file contains declared import directives, some prefixed, and (top-level) declaration. Those are what we can combine to get the top-level scopes of every file. For backwards compatibility, we want a top-level scope of each file which can contain, and will if no part file has any imports:
Anything that would break this is a no-go, it would break existing libraries with part files. That means that that's going to be the top-level scope of the library file, and the default scope of a first-level part file with no imports will be compatible with that (resolve names to the same declarations, whether its structured the same way or not). We want consistency, so adding futher nested part files with no imports should still give compatible top-level scopes in every file. Our options are limited to what adding imports in a part file changes its top-level scope, and those if its sub-parts. When you say
then because all those declarations are available as default, it means that the name should still be accessible in every file, no matter what you do with imports. Importing something with the same name as a declaration anywhere in the library will not shadow that declaration. Which leaves us at each library having all the declarations of the entire library in their top-level scope, shadowing any imports.
That's new and possible, since no part files have imports yet. It means that importing something with the same name as an inherited import, will make the name conflicted.
I think that's inconsistent with requirements and breaking, and I don't think that'll ever work. Every declaration of every file is in the top-level scope, they will shadow imports. The only way to have this restriction is that it's a compile-time error to have a declaration in any file of the library (and therefore every file of the library) with the same name as any imported declaration in any file of the library. Far too breaking. Imports tend to over-import, and if the library declares something with the same name, it has always just taken precedence, and made the imported name inaccessible.
You can't shadow inherited imports. The declartions of the current files are also in the scope of the parent file that the import came from, and would conflict there. The only thing you can possibly shadow is imports in the same file as the declaration. I think shadowing should be allowed, because it's actually useful. I generally see no advantage in disallowing shadowing or causing errors. (One alternative is to say that all import declarations in all files are applied in the library file, as its import scope, and its top-level scope is that extended with every declaration in the library. And then use that scope for every part file. But part of the reason for the design here was to give part files, fx generated by macros, the ability to safely control their scope, without risking introducing conflicts with any code anywhere else in the same library.) |
I strongly disagree that we cannot make a breaking change here. We have language versioning, and are already intending on using it. Nobody will be broken until they upgrade their language version, and we should be able to provide a I also do not anticipate that the suggested semantics would cause much breakage. It only matters if a library is using parts, and shadows a declaration from its imports, inside a part file.
I fundamentally do not think it is a good property to have. These declarations are not in the lexical scope, and so the main user intuition about scoping is gone. Beyond the lexical scope, I don't think users have a good intuition about how shadowing works at all. Thus, it is better to make the choice explicit. As a concrete example, I think it is highly confusing today that the import scope shadows the super member scope (I don't know what we actually call this scope). This should be a conflict, and require a I would like to avoid similar issues, hence allowing the lexical scope to shadow the import scope, and everything else being a conflict. Consider also that the "parent" and "children" in this case are also directly included with URI based directives, in the same way as imports. Essentially, the proposal is to put things from all URI based directives in the same scope. They are things which are external to the current file, and are all treated equally in terms of the scope. The only difference is whether they also include their private scope, and whether you are allowed to augment the declarations from those files. Concretely:
You could re-order as you please any of this except the own declaration scope, because it's conflicts all the way. Technically, we could even allow prefixes for |
We don't call it anything, because it's not a scope. If a raw identifier,
The one thing that keeps being said about this weirdness is that it causes surprisingly few problems. That said, I'd be willing to look at only allowing declarations in the current file to win over an instance member. It would mean having to look up every plain identifier that resolves to an import name in the current class, to check if it has that name (can probably be cached, if the same name is used a lot).
They are, today, because we've defined the lexical scope so that they are, the import scope and declaration scope are at the top of every lexically induced scope. (There is no difference between being "in scope" and being in the lexical scope. We always say explicitly if we use any other scope than the lexical scope, the "lexical" is mainly for emphasis.) The issue here is that there are declarations that are not syntactically visible in the file, even though they are in the lexical scope, and people tend to think of the lexical scope as only what they can see above. Imports can be seen in the library file, by their import directive, even if you can't see the names they include, but top-level declarations from other files are completely invisible. And imports are invisible in part files too. I'm not sure I agree that that's all bad. I works remarkably well, if anything, because it doesn't get in the users way most of the time. If a declaration conflicting with an import is an error, I'd have to add a lot of For the scope chain: Which declarations are in the "Parents declaration scope w/conflicts"? Is it only declarations physically in the parent file, or also declarations it includes from its sub-parts or parents? |
Ah, sorry I didn't realize how we defined the lexical scope. I always think of it only as applying to what is visibly (textually) surrounding a given location in the code. So, this is what I was referring to in all my comments above. I think people understand/expect identifiers to resolve to the "closest" declaration in the actual text surrounding that identifier. But as soon as it isn't found anywhere in that text, they don't have a good intuition about where it is looked up, in particular when it comes to the edge cases. We can certainly specify something that provides a consistent result, but it is likely that for some subset of users that won't be the intuitive result, regardless of the choice we make, because they won't all have the same intuition.
When it does cause a problem though, it can be extremely confusing. We could also use this argument as evidence that it would be perfectly fine to require explicit disambiguation here, because it will rarely actually be required.
True, it would mean some extra work. And a similar argument could be made for my proposal here - we could no longer search just the whole surrounding library for declarations, and immediately return if we find one. You have to also search all imports for any conflicting declarations, if it was found in a different "part" of the library. It feels like we must already have a pretty optimized way of doing this kind of lookup though.
It is probably worth collecting some data here - I think you and me have a different intuition for how much churn this would actually cause. My intuition is it would be very little churn. Essentially the question to answer would be, within a given library how many identifiers resolve to a declaration in that library which is shadowing a declaration by the same name from an import. If that number is large (even say 10% of libraries have a single one of these occurrences), then I would likely agree it is more annoying than it is worth. If the number turns out to be very small, let's say <1% of all libraries, would you be willing to consider my proposal, or would you still consider it too annoying? Is there a different number you would be happy with? Keep in mind, the actual effect of this would be smaller than that number, because it's only for part files where a prefix would actually be needed.
That makes sense, so I think it just becomes:
And then that answers both questions? The children's declaration scope is a bit weird to slot in, you need to skip it if it's already included in the chain previously. |
I wrote an analyzer lint to check this, which triggers any time any declaration in a library shadows one from an import, and I ran it on the flutter repo, which has 116 total violations of the lint: All violations
Many of these cases are However, the potential for breakage when adding new top level symbols to any package is definitely worth consideration. I didn't fully analyze how many of these were shadowing things from different packages, which is the main concern there (within the same package it wouldn't matter). I will see if I can also run this internally, and maybe on some actual apps as well, although it is a bit hard to get successful pub solves in the external apps I was looking at, each one needs some different version of flutter it seemed like. In general needing full analysis to produce this diagnostic makes it hard to run on a large corpus of packages/apps. |
Unfortunately I wasn't able to get the lint to apply correctly internally, not sure why 🤷♂️ . But, I think I have seen enough to be convinced that what you originally proposed is OK. It probably will do what people expect most of the time. I think ultimately I just wish shadowing wasn't a thing at all, but I am outlier in that I think. This does double down on shadowing, adding more layers of scopes and more places shadowing can happen, but 🤷♂️ . |
I am going to go ahead and close this one as well |
I specified "part files with imports" (#3800) to generalize over "augmentation libraries" and part files, so that part files can have import (etc.).
@jakemac53 had some suggestions on abilities of part files to shadow names inherited from the parent file, which differ from the current specification. The options currently on the table can be summarized as:
The four combinations would have the following top-level scopes:
Shadow inherited imports, shadow inherited declarations and only redeclared declarations from subtree:
This version can probably combine the prefix scope and the declaration scope, since we don't allow a prefix with the same name as a declaration in the declaration scope.
It's the maximally permissive combination: A part file can import any name and have it accessible as long as there is no
declaration with the same name in the part's own part-subtree. That also means that it's possible to shadow a declaration of
the same library with an import, making it inaccessible in the subtree. It's your library, knock yourself out, or use a prefix
(which also doesn't conflict with anything from a parent or sibling.)
Sibling parts can't introduce conflicting declarations, since the parent file's declaration scope must contain them all.
Shadow inherited imports, redeclare every declaration in library
This variant makes sure that library declarations are ubiquitous. The declaration scope is library wide, and
everybody has to agree on those names. That means that a declaration anywhere in the library prevents
a part file from using that name as a prefix, and shadows any import with that name.
(Solution: Import with another prefix, just like always.)
Imports, on the other hand, are just suggestions. A part file can shadow any import it inherits from its
parent with an import of its own. That allows part files (and parent files) to concentrate on the imports they
need themselves, while still allowing a library author to move any chunk of code into a new part file of
the file it came from.
Conflicting inherited imports, shadow inherited declarations and only redeclared declarations from subtree:
Where the "/w conflicts" means that if a name is imported in the part, and the same name is accessible in the parent scope
chain as an import for a different declaration, then the name is conflicted in the part file's scope. (Being conflicted is only an
error if the name is accessed.)
This allows a part file to shadow declarations from siblings (and parents), but not to ignore imports from parents.
This is more about not being obstructed by declarations in sibling libraries, while still considering every import
inherited from the parent file as intentional.
Conflicting inherited imports, redeclare every declaration in library:
This also makes the library's declarations ubiquitous, but also makes it an error to conflict with an inherited
import. It's the most restrictive variant, in that part file can conflict with any declaration anywhere, and any
import of its parent chain.
So, which version do we want, or are there more options?
@dart-lang/language-team
The text was updated successfully, but these errors were encountered: