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

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

Merged
merged 1 commit into from
Nov 7, 2024

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
4 checks passed
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