Prevent Refaster UMemberSelect from matching method parameters
#2456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stephan202
commented
Jul 18, 2021
| @@ -65,7 +65,7 @@ public static UMemberSelect create(UExpression expression, CharSequence identifi | |||
| @Override | |||
| public Choice<Unifier> visitIdentifier(final IdentifierTree ident, Unifier unifier) { | |||
| Symbol sym = ASTHelpers.getSymbol(ident); | |||
| if (sym != null && sym.owner.type != null) { | |||
| if (sym != null && sym.owner.type != null && sym.owner.type.isReference()) { | |||
There was a problem hiding this comment.
Without this change the new test throws the following exception:
memberSelectAndMethodParameterDisambiguation(com.google.errorprone.refaster.TemplateIntegrationTest) Time elapsed: 0.039 sec <<< ERROR!
java.lang.LinkageError
at com.google.errorprone.refaster.Template.callCheckMethod(Template.java:540)
at com.google.errorprone.refaster.Template.infer(Template.java:472)
at com.google.errorprone.refaster.Template.typecheck(Template.java:194)
at com.google.errorprone.refaster.ExpressionTemplate$2.apply(ExpressionTemplate.java:208)
at com.google.errorprone.refaster.ExpressionTemplate$2.apply(ExpressionTemplate.java:170)
at com.google.errorprone.refaster.Choice$2.thenOption(Choice.java:125)
at com.google.errorprone.refaster.ExpressionTemplate.unify(ExpressionTemplate.java:169)
at com.google.errorprone.refaster.ExpressionTemplate.match(ExpressionTemplate.java:128)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:110)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:54)
at jdk.compiler/com.sun.source.util.TreeScanner.visitReturn(TreeScanner.java:469)
at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCReturn.accept(JCTree.java:1554)
at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:86)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:142)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:54)
at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
at jdk.compiler/com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:248)
at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1032)
at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:86)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:142)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:54)
at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
at jdk.compiler/com.sun.source.util.TreeScanner.visitMethod(TreeScanner.java:206)
at com.google.errorprone.refaster.RefasterScanner.visitMethod(RefasterScanner.java:91)
at com.google.errorprone.refaster.RefasterScanner.visitMethod(RefasterScanner.java:54)
at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:898)
at com.google.errorprone.refaster.RefasterScanner.visitClass(RefasterScanner.java:78)
at com.google.errorprone.refaster.RefasterScanner.visitClass(RefasterScanner.java:54)
at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:808)
at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:86)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:142)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:54)
at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
at jdk.compiler/com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:144)
at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:591)
at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:86)
at com.google.errorprone.refaster.RefasterScanner.scan(RefasterScanner.java:142)
at com.google.errorprone.refaster.RefasterRule.apply(RefasterRule.java:139)
at com.google.errorprone.refaster.CodeTransformerTestHelper.transform(CodeTransformerTestHelper.java:75)
at com.google.errorprone.refaster.TemplateIntegrationTest.expectTransforms(TemplateIntegrationTest.java:68)
at com.google.errorprone.refaster.TemplateIntegrationTest.runTest(TemplateIntegrationTest.java:88)
at com.google.errorprone.refaster.TemplateIntegrationTest.memberSelectAndMethodParameterDisambiguation(TemplateIntegrationTest.java:378)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:367)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:274)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:161)
at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:290)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:242)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:121)
Caused by: java.lang.AssertionError: isSubtype METHOD
at jdk.compiler/com.sun.tools.javac.code.Types$4.visitType(Types.java:1123)
at jdk.compiler/com.sun.tools.javac.code.Types$4.visitType(Types.java:1100)
at jdk.compiler/com.sun.tools.javac.code.Types$DefaultTypeVisitor.visitMethodType(Types.java:4861)
at jdk.compiler/com.sun.tools.javac.code.Type$MethodType.accept(Type.java:1448)
at jdk.compiler/com.sun.tools.javac.code.Types$DefaultTypeVisitor.visit(Types.java:4857)
at jdk.compiler/com.sun.tools.javac.code.Types.isSubtype(Types.java:1096)
at jdk.compiler/com.sun.tools.javac.code.Types.isSubtypeUncheckedInternal(Types.java:1022)
at jdk.compiler/com.sun.tools.javac.code.Types.isSubtypeUnchecked(Types.java:1008)
at jdk.compiler/com.sun.tools.javac.code.Types.isConvertible(Types.java:607)
at jdk.compiler/com.sun.tools.javac.comp.Resolve$MethodCheckContext.compatible(Resolve.java:1021)
at jdk.compiler/com.sun.tools.javac.comp.Check.checkType(Check.java:563)
at jdk.compiler/com.sun.tools.javac.comp.Attr$ResultInfo.check(Attr.java:518)
at jdk.compiler/com.sun.tools.javac.comp.Resolve$MethodResultInfo.check(Resolve.java:1067)
at jdk.compiler/com.sun.tools.javac.comp.Resolve$4.checkArg(Resolve.java:887)
at jdk.compiler/com.sun.tools.javac.comp.Resolve$AbstractMethodCheck.argumentsAcceptable(Resolve.java:775)
at jdk.compiler/com.sun.tools.javac.comp.Resolve$4.argumentsAcceptable(Resolve.java:896)
at jdk.compiler/com.sun.tools.javac.comp.Infer.instantiateMethod(Infer.java:181)
at jdk.compiler/com.sun.tools.javac.comp.Resolve.rawInstantiate(Resolve.java:605)
at jdk.compiler/com.sun.tools.javac.comp.Resolve.checkMethod(Resolve.java:644)
at jdk.internal.reflect.GeneratedMethodAccessor32.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at com.google.errorprone.refaster.Template.callCheckMethod(Template.java:525)
... 69 more
With this change all (public) Error Prone and all Picnic-internal tests still pass (we have a test for most of our Refaster templates).
An alternative fix could be
Suggested change
| if (sym != null && sym.owner.type != null && sym.owner.type.isReference()) { | |
| if (sym != null && sym.owner.type != null && !sym.owner.type.hasTag(TypeTag.METHOD)) { |
However, the current approach seems to better convey the intent. That said, perhaps I'm missing an important case where isReference() is too restrictive.
Comment on lines
+21
to
+35
| /** | ||
| * Example template that references a field ("length") with a name which also commonly occurs as a | ||
| * method parameter name. | ||
| */ | ||
| public class MemberSelectAndMethodParameterDisambiguationTemplate<T> { | ||
| @BeforeTemplate | ||
| public int before(T[] array) { | ||
| return Objects.hashCode(array.length); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| public int after(T[] array) { | ||
| return Objects.hashCode(array); | ||
| } | ||
| } |
There was a problem hiding this comment.
Apologies for the rather uninspired test case. Internally we do have a realistic Refaster template that triggers the issue, but it depends on AssertJ, and adding a new dependency to this project didn't seem appropriate:
public final class EnumerableAssertHasSameSizeAs<S, T> {
@BeforeTemplate
EnumerableAssert<?, S> before(EnumerableAssert<?, S> enumAssert, T[] array) {
return enumAssert.hasSize(array.length);
}
@AfterTemplate
EnumerableAssert<?, S> after(EnumerableAssert<?, S> enumAssert, T[] array) {
return enumAssert.hasSameSizeAs(array);
}
}
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jul 18, 2021
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jul 23, 2021
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Aug 4, 2021
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Aug 20, 2021
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Nov 13, 2021
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jan 26, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Apr 13, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Apr 13, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Apr 15, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Apr 16, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jun 5, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Aug 4, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Oct 12, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Oct 13, 2022
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Dec 31, 2022
Stephan202
force-pushed
the
bugfix/member-select-method-parameter-disambiguation
branch
from
8d64c09 to
047d612
Compare
|
Rebased. NB: The "internal" Refaster rules and tests mentioned above have since been open sourced as part of Error Prone Support: rules, tests (executed by this class). |
Stephan202
force-pushed
the
bugfix/member-select-method-parameter-disambiguation
branch
from
047d612 to
160ec59
Compare
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jan 9, 2023
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
May 10, 2023
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
May 10, 2023
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jun 17, 2023
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Jun 17, 2023
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Aug 2, 2023
Stephan202
added a commit
to PicnicSupermarket/error-prone
that referenced
this pull request
Aug 5, 2023
cushon
approved these changes
Sep 14, 2023
Stephan202
deleted the
bugfix/member-select-method-parameter-disambiguation
branch
Stephan202
added a commit
to PicnicSupermarket/error-prone-support
that referenced
this pull request
Jan 13, 2024
Issue google/error-prone#2456 was fixed in Error Prone 2.22.0.
rickie
pushed a commit
to PicnicSupermarket/error-prone-support
that referenced
this pull request
Jan 13, 2024
Issue google/error-prone#2456 was fixed in Error Prone 2.22.0.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.