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

Extensions are not available when imported with a prefix. #671

Closed
lrhn opened this issue Nov 7, 2019 · 12 comments
Closed

Extensions are not available when imported with a prefix. #671

lrhn opened this issue Nov 7, 2019 · 12 comments
Labels
request Requests to resolve a particular developer problem

Comments

@lrhn
Copy link
Member

lrhn commented Nov 7, 2019

We designed extensions so that they are not accessible when they are imported with a prefix.

That has already lead to at least two reports of libraries which introduce their own extensions (on their own types), and which people expected to keep working when they imported the library with a prefix.

The reason to not make them accessible was that it provided a way to avoid an extension method conflict while still making explicit application possible. I don't want to lose that property.

I suggest that we loosen the extension resolution so that extensions imported with a prefix are accessible, but we give preference to extensions imported without a prefix.

The rules for extension conflict resolution for applicable extensions E1 and E2 would then be:

  • E1 is more specific than E2 if E2 is only imported through imports with a prefix, and E1 is not,
  • or both are imported only through prefix imports or neither are, and:
  • ... current rules ...

@johnniwinther WDYT?

@lrhn lrhn added the request Requests to resolve a particular developer problem label Nov 7, 2019
@johnniwinther
Copy link
Member

From the CFE perspective it is not a problem to track whether an extension was made accessible through a prefix or not; accessible extensions are tracked separately from the normal scope, anyway.

@eernstg
Copy link
Member

eernstg commented Nov 7, 2019

I think this makes sense. We might want to avoid adding extra priority rules, though.

I suspect that it happens now and then that a library L imports a bunch of other libraries, and then suddenly a version update causes a name clash because some new declarations were added. I think it would be a useful property to ensure that the refactoring where an import is changed from having no prefix to having a prefix p can be relatively simple. Except for the subtle situation where an imported name is also the name of an inherited instance member, adding the prefix will make a set of identifiers undefined, and each of them will just need to have p. added in front.

However, the situation could be much more subtle if a large number of extension method invocations may suddenly be resolved differently.

Because of this, and if we allow prefixed extensions to be used implicitly, I think it's a more robust design to maintain that the conflicts in extension resolution do not depend on prefixes.

Apart from that, I think this enhancement would be rather straightforward. For instance, I think it will work just as well with prefixes to eliminate name clashes in a prefixed namespace as it does in the library namespace:

import 'lib1.dart' as p; // Exports extension `E`.
import 'lib2.dart' as p; // Exports extension `E`.

typedef E = void Function(); // Shadow.

var x = 42.m();

So m will be able to call an extension method from one of those two extensions named E, and developers can use hide E and show E with another prefix to single out one of them as needed, such that resolution can be made explicitly at each call site.

The missing bit is that there may still be a need to have access to the name of an extension without enabling implicit invocations. Otherwise we could have many conflicts between p1.E and p2.E, where almost all of them should be resolved to p1.E. But if we hide E from p2 then we cannot even use p2.E explicitly. You could say "but then import p1.E without a prefix to make it more specific". But that violates the 1-2-infinity principle, because we might have a conflict involving 3 extensions with the same name, and we only have two priority levels.

@leafpetersen
Copy link
Member

We discussed this here this morning. I think @munificent and I are more in favor of not prioritizing un-prefixed imports during resolution, but instead treat them all the same. The prioritization feels unexpected and not especially useful. If we were designing the feature from scratch, I don't think we would have chosen to do so. Given that, I think it might be worth paying the price of a small breaking change now to get the design right. I think the odds of this actually breaking anyone are quite small, since extension methods have just soft-launched.

The only independent use we could think of for the prioritization scheme would be to allow two extensions with some conflicting members and some non-conflicting members to coincide, with the conflicting members essentially filtered out by the prioritization, and the non-conflicting members left accessible. This seems too esoteric of a use case to justify the cognitive load. If we want to support making subsets of members accessible, I think we should add explicit syntax for that.

I'm somewhat concerned that we're giving up the ability to get access to an extension's name without also making its members accessible. The general feeling here though is that this is not actually that important of an affordance in practice.

I discussed the interaction with deferred libraries with @sigmundch and @stereotype441 . It seems very user unfriendly for pieces of code that do not mention the deferred library prefix to nonetheless trigger errors if they are not preceded by a call to loadLibrary.

import "extensions_on_int.dart" deferred as p;

void main() {
 3.extensionMethod(); // Blows up at runtime, since loadLibrary has not been called
}

There are a number of possibilities for making this friendlier:

  1. Disallowing libraries with extension methods from being imported as deferred
  2. Disallowing libraries with extension methods from being imported as deferred unless all of the extensions are explicitly hidden in the import
  3. Making extension methods in deferred libraries not accessible.
    - Extension methods are still callable via the explicit syntax
  4. Making it an error if an extension method invocation resolves to an extension method from a deferred library
    - What if it's accessible from both a deferred and non-deferred import? Presumably ok?
    - Extension methods are still callable via the explicit syntax
  5. Making it a runtime error if an extension method invocation from a deferred library is invoked before loadLibrary is invoked on the prefix.
    - This is fairly user unfriendly
    - As a variant, we could also require that the relevant extension be explicitly shown in the import, to make it clearer what's going on.

@stereotype441 Indicates that 4) would be somewhat laborious to implement in the analyzer, which is a concern for getting this rolled out quickly.

If we do 1) or 2) now, then moving to 3) or 4) in the future is a non-breaking change.

If we do 3) now, then moving to 4) in the future is a breaking change, since it may cause new resolution conflicts on invocations.

@sigmundch Is concerned that 4) and 5) might be difficult to implement in the CFE as well, since it's not clear that the right information about which library an invocation came from is present. Thoughts @johnniwinther ?

A sufficient short term solution would be to do 1) or 2), and then relax this in a future release.

cc @natebosch

@lrhn
Copy link
Member Author

lrhn commented Nov 8, 2019

tl;dr:
Pro different-priority approach: Gives users the control we originally intended them to have, for the same reasons we wanted them to begin with.
Pro same-priority approach: Reduces risk of incidental changes changing priority of conflicting extensions. Open for later feature to explicitly change priority.
Pro "it doesn't really matter": The choice is only relevant in cases where there are conflicts, where the author can't solve the conflict by hiding one extensions and doesn't want to use explicit application for every conflict.

Safe approach is symmetric priority with option of adding explicit priority overrides later.
Let's do that.

The prioritization feels unexpected and not especially useful. If we were designing the feature from scratch, I don't think we would have chosen to do so.

Arguably, we did chose that.
The original choice was to allow extensions imported with a prefix to be used implicitly or not.
We chose to not allow that, not because it was a problem in itself (which is why proposing it now is not unreasonable), but because it did not give users enough control in case of a conflict.

By disallowing extension imports with a prefix from being used implicitly, we allowed a conflict to be resolved by importing one of the conflicting extensions with a prefix, and then only need to use explicit application for the prefix imported one, not both.
We chose that because we considered it better for the user than the alternative that we are now proposing to do anyway, where you have to use explicit application for all invocations.
(Or not have one of the extensions available at all, but if that was an option, the extension could just be hidden anyway).

This only matters at all in cases where there is a conflict for specific invocations, sufficiently many of them that doing explicit application for each one is something the author wants to avoid, and where they can't simply hide the other extensions. In all other situations it doesn't matter what we choose to do because there is a different and preferable solution to the conflict.
That suggest for giving them the option of actually avoiding the explicit invocation.
Or, alternatively, saying that the situation is expected to be so rare that we don't care about solving it: Just use explicit application everywhere anyway.

The best argument against the different-priority based solution, and why it differs from the original choice that we made, is that it may make some unexpected refactorings breaking. Changing an import to have (or not have) a prefix may change an existing working program into a broken one because some applicable extension methods change priority.
That does not happen with the same-priority solution because either there is a conflict, or there is a winner, and that doesn't change by adding or removing a prefix.

An argument for is that it's a safe default, and we can later add other explicit priority modifiers that are not linked to prefixing, like import "blah.dart" extension hide Foo; which would make Foo not available for implicit invocation. Such changes would be non-breaking because they are not already in use for other things, which prefix import is.

I don't think that users will have a hard time understanding the rules, but they might accidentally trip over them when doing other, otherwise unrelated, refactorings.

That did not apply, at least not in as great an extent, to the original choice because that was between extensions working and not working.
Refactoring an import into using a prefix would disable all extensions, which could still mean that other extensions would take over, but would probably just break the program immediately. With the different-priority based approach, you could still have all-but-one of an extensions methods being applied, and just one losing priority to a conflict. As such, a breaking change of semantics is easier to hide. It's a difference in degree, not in kind, but it can easily be a large degree.

@lrhn
Copy link
Member Author

lrhn commented Nov 8, 2019

About deferred imports, it's a tricky case. If we allow imports with prefixes, we no longer automatically exclude deferred imports, so we have to be explicit about them.

Deferred imported declarations are otherwise always explicit about going trough the prefix, and the user has an expectation that going through the prefix will force a check for whether the prefix has been loaded yet.
Implicit extension invocations dodges the syntactic requirement. That's not the only issue, though.

Currently deferred imports do not introduce types. You can't use the name of a class imported through a deferred import as a type. You also can't access declarations as constants, not even tear-offs of global or static functions. This should ensure that no deferred code needs to be available before the library is loaded, even though the static signatures are known at compile-time.

You can call static functions from a deferred library after loading the prefix, so there is nothing inherently preventing us from calling extensions methods. Explicitly applied extensions will use the prefix and predictably fail if the prefix has not been loaded.

The issue is implicit extensions, for a number of reasons.

If an extension is available through a non-deferred import, then it's available.
If it's only available through a single deferred import, then we can make it available after the import prefix has been loaded, as if we rewrote e.foo() to the explicit application member invocation prefix.Ext(e).foo().

That leaves the most complicated situation: Assume the extension is imported twice, through two different deferred libraries. Should it be successfully implicitly invokable when either one of the two prefixes have been loaded? Or should we pick one of the imports and be rewritten to an explicit invocation of the extension of that import: prefix1.Ext(e).foo(). Then it's arbitrary which one gets picked, which would be bad.

That situation leaves us without the option of rewriting to a concrete explicit application, which works for all non-deferred situations, and the single deferred import case. We can't do that when there are two (or more) deferred imports of the same extensions because the two imports are distinguishable. If we pick on and load the other, then the access fails.

I'd recommend a modified version 5:

  1. Allow explicit extension application for extensions imported through deferred loading. Access will be guarded by the prefix being loaded. Allow implicit extension method invocation for extensions imported only through a single deferred import, just as for other prefix-imported libraries. Again, the access will be guarded by the prefix being loaded. Make it a compile-time error to implicitly invoke an extension member which is imported only through more than one import, and only through deferred imports.

(That is: version 5 plus compile-time error in case of ambiguity between deferred imports).

I don't find option 5 that user-unfriendly. We should be able to provide a useful error message: prefix "p" not loaded for invocation of "p.Ext.foo": path/file.dart:27:13.

As for implementation: You need to know, for each accessible extension:

  • Is it available from a non-deferred import.
  • If not, which deferred prefixes are it available through (one prefix, or "more than one" is enough information).

Any explicit extension application going through a deferred prefix should have a prefix-load-guard before calling the function.
Any implicit invocation going through a single deferred prefix should behave the same and have a prefix-load-guard too.
Any implicit invocation where there is "more than one" deferred prefix is a compile-time error.

That seems doable.

@eernstg
Copy link
Member

eernstg commented Nov 8, 2019

Version 6 matches up with the (implementation) view of an extension method as a sugared syntax for a top-level function. Allowing it to be called under the same constraints makes sense.

Currently, the following crashes the front end, and seems consistent with the expectation that 'body_builder.dart' would need to take this extra case into account, and hopefully it would require only a rather local change to handle it:

// LIBRARY 'n012lib.dart'.
extension E on int {
  void get foo => print(this);
}

// LIBRARY 'n012.dart'.
import 'n012lib.dart' deferred as p;

main() async {
  await p.loadLibrary();
  p.E(1).foo;
}
type 'ExplicitExtensionAccessGenerator' is not a subtype of type 'Expression'
#0      DeferredAccessGenerator.doInvocation (package:front_end/src/fasta/kernel/expression_generator.dart:2775:25)
#1      BodyBuilder.finishSend (package:front_end/src/fasta/kernel/body_builder.dart:1462:23)
#2      PrefixUseGenerator.buildPropertyAccess (package:front_end/src/fasta/kernel/expression_generator.dart:3738:26)
#3      SendAccessGenerator.withReceiver (package:front_end/src/fasta/kernel/expression_generator.dart:4307:23)
#4      BodyBuilder.doDotOrCascadeExpression (package:front_end/src/fasta/kernel/body_builder.dart:1631:17)

@leafpetersen
Copy link
Member

Per offline discussion, we will move forward with the symmetric version. For deferred libraries, we will go with option 2 in the very short term just to give us some time to iterate on a design, and then we will relax this to some variant of one of the other options.

@lrhn
Copy link
Member Author

lrhn commented Nov 11, 2019

I have an updated specification: #672

@leafpetersen
Copy link
Member

Can we move forward with a resolution for the deferred library question? There seems to be some support behind the option 6 proposed by @lrhn : that is, make it a runtime checked error, except for a compile time error when it is ambiguous as to what deferred library is the importing library. There is the additional option to require an explicit "show" to make it clearer that something is being imported into the scope.

A concern raised by @sigmundch is that currently DDC and the VM will not signal an error if you use something from a deferred library before it is loaded. Consequently, code that is tested on DDC but shipped on dart2js may work in testing but fail in production. I believe that the behavior difference is proposed to go away soon however.

@sigmundch
Copy link
Member

... currently DDC and the VM will not signal an error if you use something from a deferred library before it is loaded.

This is the first reason I'd prefer we start with option 2. Once the behavior difference between DDC and dart2js is addressed, I would be happy moving to option 6.

Currently deferred imports do not introduce types. You can't use the name of a class imported through a deferred import as a type....

This relates to the second reason I'd like us to continue with option 2 for now.

I'd like to open the door to reconsider the restrictions with deferred imports. Today, we don't allow users to refer to a deferred type, in part because of representation limitations in our tools. In particular, in dart2js types and classes are represented by the same runtime entity, so referring to a type would have required us to have the class at hand and would have prevented us from deferring the class itself.

The context is changing a lot:

  • Soon we'll be shipping a new-rti representation in dart2js that splits classes from types. This can potentially allow us to relax that restriction.
  • Since the Dart 2 migration we also started noticing that even when users are not writing types explicitly, type inference is adding deferred types when this was not possible back in Dart 1. We have all sorts of workarounds and guidelines to prevent his from regressing deferred splits, but revisiting the restriction once the new-rti is available will allow us to be more lax and avoid workarounds.

All these changes make me more uncomfortable in charting forward with option 6 until we work out all the details.

@eernstg
Copy link
Member

eernstg commented Aug 17, 2020

@lrhn, given that extensions imported with a prefix are now accessible, is there any further work to do here or can we close this issue?

@lrhn
Copy link
Member Author

lrhn commented Aug 17, 2020

Nope, we are good. Problem solved.

@lrhn lrhn closed this as completed Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

5 participants