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

Scoping behavior of annotations disagrees with spec #1790

Closed
stereotype441 opened this issue Aug 9, 2021 · 6 comments
Closed

Scoping behavior of annotations disagrees with spec #1790

stereotype441 opened this issue Aug 9, 2021 · 6 comments
Assignees
Labels
bug There is a mistake in the language specification or in an active document

Comments

@stereotype441
Copy link
Member

The following weird program is accepted by both the analyzer and the CFE:

class A {
  const A(void Function() x);
}
class C<@A(B) T> {
  static void B() {}
}
main() {
  new C<int>();
}

This seems to contradict the spec, which says "Type parameters are declared in the type parameter scope of a class or function" (15 Generics) and "The constant expression given in an annotation is type checked and evaluated in the scope surrounding the declaration being annotated" (16 Metadata). Together these seems to imply that the reference to B in @A(B) should be an error, because there is no declaration of B in scope.

@stereotype441 stereotype441 added the bug There is a mistake in the language specification or in an active document label Aug 9, 2021
@stereotype441
Copy link
Member Author

stereotype441 commented Aug 9, 2021

Digging a little deeper, the analyzer and CFE seem to be using different mechanisms to perform the (IMHO erroneous) resolution of B. The analyzer seems to resolve it via an implicit reference to this (which seems wrong, because this isn't available in annotations), and the CFE seems to resolve it via the class scope (which seems wrong, because the annotation is outside the class body). You can see the difference if you try analyzing/compiling this example:

class A {
  const A(void Function() x);
}
class B {}
class C<@A(B) T extends B> {
  static void B() {}
}
main() {
  new C<B>();
}

This example is accepted by the CFE, but rejected by the analyzer. The analyzer gives this error message:

error • test.dart:5:12 • The argument type 'Type' can't be assigned to the parameter type 'void Function()'. • argument_type_not_assignable

The fact that the CFE accepts this code strongly suggests that it is interpreting the B in @A(B) as referring to static void B() (otherwise there would be a type mismatch in the call to A's constructor), but it is interpreting the B in extends B as referring to class B (otherwise new C<B>() would not work).

The analyzer's error message strongly suggests that it is interpreting the B in both @A(B) and extends B as referring to class B.

My understanding of the spec is that the analyzer's interpretation is correct.

@stereotype441
Copy link
Member Author

@eernstg Is my interpretation of the spec correct here? I believe the first example should be an unresolved identifier error (because there is no B in scope at the site of @A(B)), and the second example should be a type mismatch error (because the B in @A(B) should be interpreted as referring to class B)

@stereotype441
Copy link
Member Author

Note: here is a further illustration that the analyzer is resolving annotations on type arguments via implicit this:

class A {
  const A(void Function() x);
}
class C {
  static void B() {}
}
extension E<@A(B) T> on C {}
main() {}

The analyzer reports no error for this, because the B in @A(B) is resolved to the static method C.B. The CFE rejects it, reporting Error: Getter not found: 'B'.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Aug 9, 2021
Currently, if a type parameter of a class, mixin, or extension
declaration has an annotation, that annotation is resolved in the type
parameter scope of the class, mixin, or extension declaration (meaning
that static methods are not in scope), however if scope lookup fails,
then static methods are accessible via implicit `this`.

AFAICT, this odd behavior is not spec compliant, so I've filed
dart-lang/language#1790 to discuss the
possibilty of changing it.  However, for now I want to unit test the
behavior in order to ensure that if we make any changes to the
analyzer that affect it, the change won't go unnoticed.

Bug: dart-lang/language#1790
Change-Id: I02df7e6e8939ecc6222085f28a6d6b7d072179c1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/209662
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
@eernstg
Copy link
Member

eernstg commented Aug 10, 2021

Good catch! ;-)

the first example should be an unresolved identifier error ..
the second example should be a type mismatch error

Agreed.

@stereotype441
Copy link
Member Author

In discussion with @leafpetersen the other day, we decided that it's extremely unlikely that anyone is taking advantage of this scoping bug, so there's no need to tie the fix to a particular language version to assist folks in migrating. We can just fix it, provided that we make a note in the changelog.

I just ran an experimental fix (https://dart-review.googlesource.com/c/sdk/+/209940/2) through google3 and found no failures, confirming that no one is taking advantage of this bug anywhere in internal sources. So I'm going to proceed with the full bug fix.

@stereotype441 stereotype441 self-assigned this Aug 12, 2021
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Aug 17, 2021
Previously, when encountering identifiers in metadata on a class's
type parameter, the analyzer would resolve them using the type
parameter scope, but then fall back on using implicit `this`.  The CFE
would resolve them using the class body scope.  In both cases, the end
result was that the annotation could refer to static class members.

This change brings the behavior of both the analyzer and the CFE in
line with the spec, by preventing the use of implicit `this` in these
annotations, and resolving them in the type parameter scope.

This is not expected to break any code in practice, because
annotations on type parameters are rare, as are annotations referring
to static class members, and the overlap between these two should be
negligibly small.

Fixes dart-lang/language#1790.

Bug: dart-lang/language#1790
Change-Id: Ibe5a421e04a53d29074a8b1509e1390658ed72e5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210040
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
@stereotype441
Copy link
Member Author

Fixed by dart-lang/sdk@f5a3bce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug There is a mistake in the language specification or in an active document
Projects
None yet
Development

No branches or pull requests

2 participants