Bad replacement suggestions

cushon commented 9 years ago

Original issue created by mdempsky@google.com on 2013-10-09 at 09:57 PM

I've written this checker to detect when people try to use GWT's thirdparty classes instead of using the same classes directly:

<<<<<<<< @BugPattern(name = "GWTThirdParty", summary = "Found use of com.google.gwt.thirdparty.", explanation = "You should use thirdparty libraries directly instead of using GWT's"

" internal repackaged jars.", linkType = NONE, category = ONE_OFF, severity = WARNING, maturity = EXPERIMENTAL) public class ThirdPartyChecker extends BugChecker implements MemberSelectTreeMatcher { @Override public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) { Symbol sym = ((JCFieldAccess) tree).sym; if (!(sym instanceof PackageSymbol)) { return Description.NO_MATCH; } switch (sym.toString()) { case "com.google.gwt.thirdparty.guava": return describeMatch(tree, new SuggestedFix().replace(tree, "com.googleX")); default: return Description.NO_MATCH; } } }

but when I apply it to this test file:

<<<<<<<< import com.google.gwt.thirdparty.guava.common.base.Joiner;

/**

Test case for {@link ThirdPartyChecker}. */ public class ThirdPartyCheckerTest { static { com.google.gwt.thirdparty.guava.common.base.Splitter.on("x"); } }

I get this output:

<<<<<<<< .../ThirdPartyCheckerTest.java:3: warning: [GWTThirdParty] Found use of com.google.gwt.thirdparty. import com.google.gwt.thirdparty.guava.common.base.Joiner; ^ Did you mean 'import com.googleX.common.base.Joiner;'? .../ThirdPartyCheckerTest.java:10: warning: [GWTThirdParty] Found use of com.google.gwt.thirdparty. com.google.gwt.thirdparty.guava.common.base.Splitter.on("x"); ^ Did you mean 'com.googleX.gwt.thirdparty.guava.common.base.Splitter.on("x");'? 2 warnings

I.e., the first replacement is okay, but the second is wrong.

If I change the replacement string to "com.googleXX", then I get:

<<<<<<<< [...]/ThirdPartyCheckerTest.java:3: warning: [GWTThirdParty] Found use of com.google.gwt.thirdparty. import com.google.gwt.thirdparty.guava.common.base.Joiner; ^ Did you mean 'import com.googleXX;'? [...]/ThirdPartyCheckerTest.java:10: warning: [GWTThirdParty] Found use of com.google.gwt.thirdparty. com.google.gwt.thirdparty.guava.common.base.Splitter.on("x"); ^ Did you mean 'com.googleXX.thirdparty.guava.common.base.Splitter.on("x");'? 2 warnings

I.e., now both suggestions are wrong. Curiously, if I change "com.googleXX" to "com.googleXY" or something, then I get the previous half-right behavior.

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-09 at 10:18 PM

Hrm, I'm suspecting this is a limitation due to javac. It doesn't look like JavacParser records end positions for member select tree nodes. :(

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-09 at 10:36 PM

Blah, yeah, I changed the matcher logic to:

<<<<<<<< @Override public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) { JCFieldAccess fa = (JCFieldAccess) tree; Symbol sym = TreeInfo.symbol(fa.selected); if (!(sym instanceof PackageSymbol)) { return Description.NO_MATCH; } switch (sym.toString()) { case "com.google.gwt.thirdparty.guava": return describeMatch(tree, new SuggestedFix().replace( fa.getStartPosition(), fa.getPreferredPosition(), "com.google")); default: return Description.NO_MATCH; } }

I.e., now I'm matching on a member select like "com.google.gwt.thirdparty.guava.common" and then take advantage of fa.getPreferredPosition() giving the location of the '.' before common), so I can replace from the start up to the dot, and now it works as intended:

<<<<<<<< .../ThirdPartyCheckerTest.java:3: warning: [GWTThirdParty] Found use of com.google.gwt.thirdparty. import com.google.gwt.thirdparty.guava.common.base.Joiner; ^ Did you mean 'import com.google.common.base.Joiner;'? .../ThirdPartyCheckerTest.java:10: warning: [GWTThirdParty] Found use of com.google.gwt.thirdparty. com.google.gwt.thirdparty.guava.common.base.Splitter.on("x"); ^ Did you mean 'com.google.common.base.Splitter.on("x");'? 2 warnings

So there's an acceptable workaround here (at least IMO) and fixing this properly would probably require some cooperation from langtools, so I'm going to go ahead and close as WontFix.

Status: WontFix

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-10 at 05:02 AM

Hm, I'm not so sure now. I just wrote this test program using [almost] only officially supported Javac APIs to test what end positions it prints out:

<<<<<<<< import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.MemberSelectTree; import com.sun.source.util.JavacTask; import com.sun.source.util.SourcePositions; import com.sun.source.util.TreePath; import com.sun.source.util.TreePathScanner; import com.sun.source.util.Trees;

import java.io.IOException; import java.util.Arrays; import java.util.HashSet; import java.util.Set;

import javax.lang.model.element.Element; import javax.lang.model.element.PackageElement; import javax.tools.JavaCompiler; import javax.tools.JavaFileObject; import javax.tools.StandardJavaFileManager; import javax.tools.ToolProvider;

public class Bug { public static void main(String[] args) throws IOException { JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); try (StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, null, null)) { Iterable<? extends JavaFileObject> files = fileManager.getJavaFileObjectsFromStrings(Arrays.asList(args)); JavacTask task = (JavacTask) compiler.getTask(null, fileManager, null, null, null, files); Trees trees = Trees.instance(task); Set<CompilationUnitTree> units = new HashSet<>(); for (Element element : task.analyze()) { units.add(trees.getPath(element).getCompilationUnit()); } for (CompilationUnitTree unit : units) { scan(unit, trees); } } }

private static void scan(final CompilationUnitTree unit, final Trees trees) throws IOException { final CharSequence source = unit.getSourceFile().getCharContent(false);

new TreePathScanner<Void, Void>() {
  @Override
  public Void visitMemberSelect(MemberSelectTree node, Void arg) {
    Element element = getElement(trees, getCurrentPath());
    if (element instanceof PackageElement) {
      SourcePositions positions = trees.getSourcePositions();
      long start = positions.getStartPosition(unit, node);
      long end = positions.getEndPosition(unit, node);

      System.out.println(node);
      System.out.println(start + ":" + end);
      System.out.println(source.subSequence((int) start, (int) end));
      System.out.println();
    }

    return super.visitMemberSelect(node, arg);
  }
}.scan(unit, null);

}

private static Element getElement(Trees trees, TreePath path) { Element element = trees.getElement(path); if (element == null) { /* * Before langtools 8, {@link Trees#getElement(TreePath)} did not handle * {@link MemberSelectTree} trees. / element = com.sun.tools.javac.tree.TreeInfo.symbol( (com.sun.tools.javac.tree.JCTree) path.getLeaf()); } return element; } }

But when I run it against itself, I get correct outputs; e.g., the first few lines of output are:

<<<<<<<< com.sun.source.tree 7:26 com.sun.source.tree

com.sun.source 7:21 com.sun.source

com.sun 7:14 com.sun

com.sun.source.tree 55:74 com.sun.source.tree

com.sun.source 55:69 com.sun.source

com.sun 55:62 com.sun

So this does seem to be an error-prone bug after all. =/

Status: Accepted CC: eaftan@google.com

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-10 at 07:05 AM

Some further testing, I find that compiling with -Xjcov gives me correct recommendations, but turning it back off breaks them again. So I suspect there's an issue in the re-parse and/or WrappedTreeMap logic.

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-10 at 09:57 AM

Aha, the bug was introduced in commit 8b3cdede. (I kept staring at commit c2d430b trying to figure out what could have been wrong.)

The issue is that WrappedTypeMap (since 8b3cdede) considers trees to be equal if their (startPosition, kind, tag) are equal. But in my case, the memberselects "foo.bar" and "foo.bar.quux" will be equal in all of those, so WrappedTypeMap will just semi-randomly pick one to return.

This can happen for other expressions too. E.g., in "a + b + c", we'll randomly confuse the end positions immediately after 'b' and after 'c'.

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-10 at 10:56 AM

This issue was closed by revision 667a06867284.

Status: Fixed

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-10 at 10:58 AM

I've uploaded a fix to branch mdempsky/fix200: https://code.google.com/p/error-prone/source/detail?r=667a06867284

I've verified that if I simply enable the RuntimeException in WrappedTreeMap's constructor, then EndPosTest fails because of "System.out" and "System.out.println" colliding. With the rest of the change (i.e., the preferred position checks) it passes again.

Please review and I'll merge to master.

Status: Started Owner: eaftan@google.com CC: -eaftan@google.com, mdempsky@google.com

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-25 at 07:12 PM

Moved review to http://codereview.appspot.com/17260043

CC: supertri@google.com

cushon commented 9 years ago

Original comment posted by mdempsky@google.com on 2013-10-31 at 07:10 PM

This issue was closed by revision eaf8bed058b7.

Status: Fixed

cushon / issues-import

Bad replacement suggestions #200