Skip to content

Ignore all annotations in all scopes if source is missing#10021

Merged
niloc132 merged 1 commit intogwtproject:mainfrom
niloc132:10020-bytecode-annotations-test
Nov 7, 2024
Merged

Ignore all annotations in all scopes if source is missing#10021
niloc132 merged 1 commit intogwtproject:mainfrom
niloc132:10020-bytecode-annotations-test

Conversation

@niloc132
Copy link
Member

JDT supports three kinds of annotations, and the ASTVisitor lets them be encountered in two different scopes. This patch ensures that all six cases are handled, and adds a test that uses each of the six cases - removing any one of the visit implementations will fail the test by incorrectly reporting the annotation as not having sources available.

Fixes #10020

@niloc132 niloc132 added this to the 2.13 milestone Oct 29, 2024
String code = Joiner.on('\n').join(
"package test;",
"import com.google.gwt.dev.jjs.impl.*;",
"public class EntryPoint<T extends " + annotations + " Object> {",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we go all-in here and add annotations everywhere allowed in source code so the test is basically a catch all test and not tailored to the available methods of JDT ASTVisitor (BinaryTypeReferenceVisitor)?

JDT knows additional scopes (Module, CompilationUnit, Method) and if we update JDT some visitor might change and we might not detect it here given visitors have default implementations. For example block scope could become method scope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth considering, but the ASTVisitor doesn't check those, so the particular code in question that is actively looking for binary type refs so that it can complain about them in the logs (and throw an exception) isn't going to fail, at least as I read it.

Would you contribute a test that does that? I'm guessing there is an example out there of all the different ways you can stack annotations that can be used to extend this. We'd need (at least) three instances of every location, to hit marker/normal/single.

@niloc132 niloc132 merged commit c2229e7 into gwtproject:main Nov 7, 2024
niloc132 added a commit to niloc132/gwt that referenced this pull request Nov 7, 2024
…#10021)

JDT supports three kinds of annotations, and the ASTVisitor lets them be
encountered in two different scopes. This patch ensures that all six
cases are handled, and adds a test that uses each of the six cases -
removing any one of the `visit` implementations will fail the test by
incorrectly reporting the annotation as not having sources available.

Fixes gwtproject#10020
niloc132 added a commit that referenced this pull request Nov 11, 2024
JDT supports three kinds of annotations, and the ASTVisitor lets them be
encountered in two different scopes. This patch ensures that all six
cases are handled, and adds a test that uses each of the six cases -
removing any one of the `visit` implementations will fail the test by
incorrectly reporting the annotation as not having sources available.

Fixes #10020
See #10021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing sources for type annotations trip the compiler

3 participants