Accessing privates through a prefixed import of the current library doesn't work

[jakemac53]

It is possible this is working as intended, so let me know if so. I am not sure exactly what the next course of action would be though in that case.

Currently the functional widget example macro adds a reference to the original function, which it requires to be private, using the identifier. This ends up adding an import to the current library with a prefix, full example is like this:

main library

@FunctionalWidget()
Widget _myApp(BuildContext context) {...}

augmentation

library augment 'package:macros_example/main.dart';

import 'package:flutter/src/widgets/framework.dart' as prefix0;
import 'dart:core' as prefix1;
import 'package:macros_example/main.dart' as prefix2;

class MyApp extends prefix0.StatelessWidget {
  const MyApp({super.key,});
  @prefix1.override
  prefix0.Widget build(prefix0.BuildContext context) => 
    prefix2._myApp(context, );
}

This gives an error that prefix2._myApp does not exist.

If I change the generator to just output the name of the private function, it does work, since it doesn't get prefixed.

[keertip]

/cc @bwilkerson @scheglov

[bwilkerson]

I believe the correct answer to this issue is that the analyzer needs to conform to the specification.

Which begs the next question: Is this behavior (being able to reference private variables in the augmented library via an import) documented in the specification for library augmentations? If not, and assuming that we want to support this, then it should be.

[scheglov]

FWIW, VM also does not allow this:

import 'test.dart' as self;

void _foo() {
  print('_foo');
}

main() {
  self._foo();
}

...produces:

bin/test.dart:8:8: Error: Method not found: '_foo'.
  self._foo();
       ^^^^

[scheglov]

Quotes from the specification. 19.1.1 The Imported Namespace

19.2 Exports

So, only public top-level declarations are visible through imports.

[bwilkerson]

Which suggests that if we want to allow this in library augmentations we might consider making it a special case that isn't applied to all imports, but only when importing the augmented library.

[jakemac53]

cc @lrhn @munificent @eernstg for thoughts on the specification

From a naive perspective, if a library imports itself it should have access to the private namespace.

If necessary though, it wouldn't be too hard to omit these prefixes for private identifiers. The situations where the prefix would actually disambiguate anything are quite rare (only in cases of shadowing with a local variable or something).

[jakemac53]

At least in the short term it seems to me like updating the augmentation library generation code to not emit a prefix for private identifiers is the best move.

[munificent]

From a naive perspective, if a library imports itself it should have access to the private namespace.

This isn't a corner of the language I'm super familiar with, but from skimming the spec and thinking about it some, I believe it would work OK to say that importing a library into the same library (with or without a prefix) includes the private members of the library.

If you do that with an unprefixed import, it wouldn't change any of the semantics that are available today as far as I can tell. The language already handles collisions with public names when a library imports itself and treats those as non-conflicting. Extending that to include private names should work fine.

Prefixed imports currently don't allow private names at all, so there is no existing code like prefix._foo which could be broken. Nor is there any code like prefix._foo which currently resolves to something else but would end up shadowed by an imported private name from the same library, since shadowing only applies to bare identifiers, not qualified ones.

So I believe we could just say that when a library imports itself, the imported namespace includes the private declarations as well as the public ones. Obviously, that wouldn't apply if a library exports itself.

I'd be OK with changing the language to allow that, and then augmentations can reliably use a prefixed import of the main library to refer to declarations from it and avoid risking shadowing.

[lrhn]

It's working as intended.

I believe it would work OK to say that importing a library into the same library (with or without a prefix) includes the private members of the library.

It probably wouldn't be a problem to say so, but the current specification only includes public names in the export scope. It would be a change to include private names too, and it would require a rewrite of some parts of the spec which currently assume that if two different imports have the same name, they conflict. Scope lookup would have to include "is accessible" checks that are currently unnecessary. (The specification does not use name mangling, the declarations have the same name, but at most one of them can be accessible in any given library. Which means that the complexity is at the lookup, not the declaration.)

Not impossible, but more work than not doing it.

If private names are not prefixed, they should still resolve to the correct name. Well, unless shadowed. But that shadowing happens inside the same library, and declaration, what has the reference. Just make that an error, so the author stops shadowing their own private names. Use fresh names for fresh privates in macros.

(An unprefixed import of the same library wouldn't work because of conflict resolution, but because the declarations shadow any import with the same name. Conflict resolution is only between imports.)

[lrhn]

Alternatively, let the augmentation name its parent file:

augmentation of "foo.dart" as foo;

Then refer to the scope of foo.dart through foo.

[munificent]

It probably wouldn't be a problem to say so, but the current specification only includes public names in the export scope. It would be a change to include private names too,

Yeah, that's what I'm proposing. When a library imports itself the export scope that it imports would be expanded to include (its own) private declarations.

and it would require a rewrite of some parts of the spec which currently assume that if two different imports have the same name, they conflict.

I don't think so. From my (admittedly quick) reading of the spec, it already says it's not a collision error if the two imports have a name collision but the names refer to the same underlying declaration. That's necessary to make this not an error:

// a.dart
int foo = 1;

// b.dart
export 'a.dart';

// c.dart
export 'a.dart';

// d.dart
import 'b.dart';
import 'c.dart';

main() {
  print(foo); // No error.
}

There is a name collision on the import scopes of b and c into d, but there's no error because the colliding names refer to the same underlying declaration.

Scope lookup would have to include "is accessible" checks that are currently unnecessary. (The specification does not use name mangling, the declarations have the same name, but at most one of them can be accessible in any given library. Which means that the complexity is at the lookup, not the declaration.)

For unprefixed imports, you either:

1. Are importing your own library, in which case you are importing your own private declarations into your own top-level namespace... where they already are. So this is entirely a no-op.
2. Are importing another library, in which case you don't get the private names at all, just like today.

For a use of a prefix, either:

1. The prefix refers to your own library, in which case we allow private names after the prefix and they resolve to your own private declarations.
2. The prefix refers to another library, in which case it's an error to have any private name after the prefix, as it is today.

I'm assuming that we would not allow exports to export the private names of a library. So if you do:

// a.dart
import 'b.dart' as b;

int _foo = 1;

main() {
  print(b._foo);
}

// b.dart
export 'a.dart'

There would be no way to see a private name if you indirectly import yourself. (Because, really, who would do this?!)

Not impossible, but more work than not doing it.

True of so many things!

But that shadowing happens inside the same library, and declaration, what has the reference. Just make that an error, so the author stops shadowing their own private names. Use fresh names for fresh privates in macros.

I definitely see the appeal of keeping it simple. But we are trying to give users a reliable story that a macro can generate code that contains a reference to some already-resolved identifier and the resulting code will actually faithfully preserve that resolution.

We could potentially add a syntax to the language for "fully qualified identifiers". But import prefixes are already so close to that that it seems gratuitous to add a separate feature. Basically, all we really need is ::_foo, where the :: as in C++ means, "skip any surrounding lexical namespaces and jump straight to the top level". An import of your own library with a prefix actually works pretty well today as an implementation of ::... except for private names.

[lrhn]

For unprefixed imports, you either:

1. Are importing your own library, in which case you are importing your own private declarations into your own top-level namespace... where they already are. So this is entirely a no-op.
2. Are importing another library, in which case you don't get the private names at all, just like today.

Ad 1. It's imported into the import scope. Then that's shadowed by the declaration scope, which means there is not way to access the imported name. Unless it's imported with a prefix.

Ad 2. That's the problem. If libraries export private names (private names are part of their export scope), then you also import private names, unless we do something about it. Not saying we can't do anything, just that we need to change the specification to account for that. Currently it can be oblivious to importing private names because private names aren't exported.

Maybe the solution is to just say at the import that any declaration in the imported library's export scope that is not accessible to the importing library, is not imported. Just as if it was implicitly hidden (but I prefer to not use "just as" definitions, so it's just not imported. It's not there. It's a goner.). And maybe that's what you've been saying all along, I just need to see it explicitly :)

That avoids complications like importing private extension declarations with public members, which then apply because they're in the imports, even if you can't name them. That's exactly the kind of complications we need to make sure we avoid.

For a use of a prefix, either:

1. The prefix refers to your own library, in which case we allow private names after the prefix and they resolve to your own private declarations.
2. The prefix refers to another library, in which case it's an error to have any private name after the prefix, as it is today.

Except that you can import multiple libraries into the same prefix:

library foo;
import "foo.dart" as prefix; // Me!
import "bar.dart" as prefix; // Not me!

final String _foo = "Huzzah!";
void main() {
  print(prefix._foo);
}

If bar.dart exports its own _foo declaration (which it does if it's able to import it itself), then we need to specify what prefix._foo means, without assuming name mangling.

That's definitely doable. If we can get away with just saying that the import ignores inaccessible declarations (and as you say, exports should too), then it might even be easy, because then it's impossible to have a conflict, there can be at most one accessible private-named declaration for any private name and library. And the change to the current semantics ends precisely one step after where the change started, so the rest should be unaffected.

That gets us back to the original use of this: A library augmentation importing its own library. That should be no different from the library importing it, but the export scope of a library doesn't exist until its declaration scope is complete, after augmentations have all been applied. That means that the augmentation library would be able to refer to the parent library import prefix in function bodies, maybe even in types, but it cannot be allowed to do:

import "ownlibrary.dart" as prefix;
augment class prefix.Foo { ... }

because prefix.Foo doesn't exist, because the export scope doesn't exist, at the point where the augmentation is applied. We may be able to say that a declaration of that name exists, but we can't say which declaration it is.

I also can't see why it would need to do that, so it's probably not a problem. If all the prefix is needed for is to refer to the top-level scope even if it's shadowed, then this should work. (It already works for non-private names, I've used it.)

But we could consider giving libraries an easier way to refer to themselves, like:

library as someName;

which would allow the library to refer to a top-level declaration, in the library's declaration scope, as someName.Declaration, but including private names. Then the library augmentation could do augment library "foo.dart" as someName; too, to refer to the same declaration scope.

[munificent]

That means that the augmentation library would be able to refer to the parent library import prefix in function bodies, maybe even in types, but it cannot be allowed to do:

import "ownlibrary.dart" as prefix;
augment class prefix.Foo { ... }

because prefix.Foo doesn't exist, because the export scope doesn't exist, at the point where the augmentation is applied. We may be able to say that a declaration of that name exists, but we can't say which declaration it is.

Yes, that should definitely be disallowed. I think this is implicit from the grammar, which doesn't allow a prefixed name for the name of the class being augmented. Augmentations are always matched up syntactically purely by bare identifier name because, as you note, we need to be able to do that before identifiers are resolved.

[davidmorgan]

What happens to this issue with the planned unification on parts? https://github.com/dart-lang/language/issues/3848

[lrhn]

No change to this issue's reason to exist due to using part for everything. It's all the same, only the labels change.

[davidmorgan]

Thanks.

It's not clear to me how much work needs doing / by whom / by when?

I'm trying to attach "t-shirt sizes" to the open macro issues: S, M, L, XL ... :)

[lrhn]

Should probably ask the CFE team, they'd be doing all the work. It's a new language feature, with all that entails of versioning and experiment flags. That's mostly project management, so S. (Anything I don't understand must be easy, right?)

Then it all depends on how may assumptions baked into the code that this change breaks.

Maybe it's just a matter of changing && !declaration.isPrivate to && (importingSameLibrary || !declaration.isPrivate) in one place, and then everything just works.

Maybe it requires going over all import code and looup code with a fine-toothed comb to check if anything makes an assumption like if (name.isPrivate) return null; // Can't be in the import scope. Impossible to say from the outside, so we should ask the insiders.

@johnniwinther?

[johnniwinther]

The omission of private names in export scopes is heavily baked in to the encoding and handling of imports/exports. Currently everything is defined and passed using String and this would have to change to use the Name class which holds a reference to the library. For instance the name of classes are just Strings for this reason.

[eernstg]

I like the proposal from @lrhn about naming the current library in order to enable top-level and static member access in spite of shadowing declarations in intermediate scopes.

I've created a small proposal taking this idea a couple of steps further: https://github.com/dart-lang/language/issues/3899.