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

Prevent Refaster UMemberSelect from matching method parameters #2456

Conversation

Stephan202
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Jul 18, 2021
Copy link
Contributor Author

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Added some context. The diff looks larger than it is: it's a one-line fix; the remainder constitutes a single Refaster template test.

@@ -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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Stephan202 force-pushed the bugfix/member-select-method-parameter-disambiguation branch from 8d64c09 to 047d612 Compare December 31, 2022 11:35
@Stephan202
Copy link
Contributor Author

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 Stephan202 force-pushed the bugfix/member-select-method-parameter-disambiguation branch from 047d612 to 160ec59 Compare January 8, 2023 16:39
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
copybara-service bot pushed a commit that referenced this pull request Sep 14, 2023
Fixes #2456

FUTURE_COPYBARA_INTEGRATE_REVIEW=#2456 from PicnicSupermarket:bugfix/member-select-method-parameter-disambiguation 160ec59
PiperOrigin-RevId: 565455682
copybara-service bot pushed a commit that referenced this pull request Sep 14, 2023
Fixes #2456

FUTURE_COPYBARA_INTEGRATE_REVIEW=#2456 from PicnicSupermarket:bugfix/member-select-method-parameter-disambiguation 160ec59
PiperOrigin-RevId: 565455682
@Stephan202 Stephan202 deleted the bugfix/member-select-method-parameter-disambiguation branch September 15, 2023 04:58
Stephan202 added a commit to PicnicSupermarket/error-prone-support that referenced this pull request Jan 13, 2024
rickie pushed a commit to PicnicSupermarket/error-prone-support that referenced this pull request Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants