Skip to content

[cfe] LibraryIndex throws when getting instance members that have name collisions with named constructors #59910

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

Open
nshahan opened this issue Jan 15, 2025 · 6 comments
Assignees
Labels
legacy-area-front-end Legacy: Use area-dart-model instead.

Comments

@nshahan
Copy link
Contributor

nshahan commented Jan 15, 2025

If you have a simple class like this:

class C {
  int foo = 10;
  C.foo();
}

and compile it to a component, then try to access the field via a library index:

var index = LibraryIndex(component, ['example.dart']);
index.getField('example.dart', 'C', 'foo');

You get an error message like this:

  Member 'foo' in class 'C' in library 'example.dart' is not a Field: C.foo (Constructor).

cc @jensjoha

@johnniwinther
Copy link
Member

Weirdly, the spec allows instance members and constructors with the same name (but not static members and constructors with the same name) - a fact that I wasn't aware of until recently. It might be that the library index also assumes that we cannot have instance members and constructors of the same name.

@eernstg
Copy link
Member

eernstg commented Jan 16, 2025

I think the motivation for allowing

class A {
  A.foo();
  void foo() {}
}

was that there is no location in code where the constructor is denoted by foo. The name of the constructor is A.foo. Similarly, there's no location where the instance member is denoted by A.foo and I don't see how we could reasonably want to introduce a way to denote the instance member as A.foo. So there is no name clash.

For a static member and an instance member there is a name clash. This occurs already at the fundamental scoping level because static void bar() {} and void bar() {} in the same class is a name clash in the body scope of that class. Granted, @lrhn wants to allow this, and it is possible to disambiguate all call sites by using A.bar to refer to the static member. I'd prefer to avoid playing around with name clashes even if they can be disambiguated, though.

@FMorschel
Copy link
Contributor

There would be a name clash in dart docs (I think?)

What would happen when you do:

/// Text for [A.foo]

@srawlins

@eernstg
Copy link
Member

eernstg commented Jan 16, 2025

Good point, it does make sense to refer to the declaration as such using A.foo, even though A.foo doesn't make sense as an expression (unless and until we add something like the C++ 'pointer to member' mechanism).

However, it's a breaking change to make it a name conflict in the language. Perhaps it should just be handled by the DartDoc tool by adding some special syntax to disambiguate this case?

@srawlins
Copy link
Member

Dartdoc prefers the A.foo instance member. To specifically refer to the constructor, you can use A.foo(). We should document this behavior here: https://dart.dev/tools/doc-comments/references

@lrhn
Copy link
Member

lrhn commented Jan 17, 2025

So if you add () to a name, DartDoc will disambiguate it to the constructor rather than an instance method.

We'll need more disambiguation if we allow static and instance members with the same name. Especially if the static one is a getter, where the () do not make sense. Or both are getters, so () can't pick the one where it does make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-front-end Legacy: Use area-dart-model instead.
Projects
None yet
Development

No branches or pull requests

7 participants