Attachment 158758 Details for Bug 235253 – Proposed fix + regression tests

[patch] Proposed fix + regression tests

patch_235253_194358.txt (text/plain), 20.28 KB, created by Olivier Thomann

on 2010-02-10 14:10:10 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Olivier Thomann

Created: 2010-02-10 14:10:10 EST

Size: 20.28 KB

patch

obsolete

>### Eclipse Workspace Patch 1.0
>#P org.eclipse.jdt.core
>Index: dom/org/eclipse/jdt/core/dom/rewrite/ImportRewrite.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/rewrite/ImportRewrite.java,v
>retrieving revision 1.18
>diff -u -r1.18 ImportRewrite.java
>--- dom/org/eclipse/jdt/core/dom/rewrite/ImportRewrite.java	21 Jan 2010 16:46:38 -0000	1.18
>+++ dom/org/eclipse/jdt/core/dom/rewrite/ImportRewrite.java	10 Feb 2010 19:03:28 -0000
>@@ -24,6 +24,7 @@
> import org.eclipse.jdt.core.ICompilationUnit;
> import org.eclipse.jdt.core.IImportDeclaration;
> import org.eclipse.jdt.core.ITypeRoot;
>+import org.eclipse.jdt.core.JavaCore;
> import org.eclipse.jdt.core.JavaModelException;
> import org.eclipse.jdt.core.Signature;
> import org.eclipse.jdt.core.compiler.CharOperation;
>@@ -137,6 +138,7 @@
> 	private String[] createdStaticImports;
> 
> 	private boolean filterImplicitImports;
>+	private boolean useContextToFilterImplicitImports;
> 
> 	/**
> 	 * Creates a {@link ImportRewrite} from a {@link ICompilationUnit}. If <code>restoreExistingImports</code>
>@@ -221,6 +223,8 @@
> 			this.restoreExistingImports= false;
> 		}
> 		this.filterImplicitImports= true;
>+		// consider that no contexts are used
>+		this.useContextToFilterImplicitImports = false;
> 
> 		this.defaultContext= new ImportRewriteContext() {
> 			public int findInContext(String qualifier, String name, int kind) {
>@@ -303,15 +307,41 @@
> 	}
> 
> 	/**
>-	 * Specifies that implicit imports (types in default package, package <code>java.lang</code> or
>-	 * in the same package as the rewrite compilation unit should not be created except if necessary
>-	 * to resolve an on-demand import conflict. The filter is enabled by default.
>-	 * @param filterImplicitImports if set, implicit imports will be filtered.
>+	 * Specifies that implicit imports (for types in <code>java.lang</code>, types in the same package as the rewrite
>+	 * compilation unit, and types in the compilation unit's main type) should not be created, except if necessary to
>+	 * resolve an on-demand import conflict.
>+	 * 
>+	 * The filter is enabled by default.
>+	 * 
>+	 * 
>+	 * Note: {@link #setUseContextToFilterImplicitImports(boolean)} can be used to filter implicit imports
>+	 * when a context is used.
>+	 * 
>+	 * 
>+	 * @param filterImplicitImports
>+	 * if <code>true</code>, implicit imports will be filtered
>+	 * 
>+	 * @see #setUseContextToFilterImplicitImports(boolean)
> 	 */
> 	public void setFilterImplicitImports(boolean filterImplicitImports) {
> 		this.filterImplicitImports= filterImplicitImports;
> 	}
> 
>+	/**
>+	 * Sets whether a context should be used to properly filter implicit imports.
>+	 * 
>+	 * By default, the option is disabled to preserve existing behavior.
>+	 * 
>+	 * 
>+	 * @param useContextToFilterImplicitImports the given setting
>+	 * 
>+	 * @see #setFilterImplicitImports(boolean)
>+	 * @since 3.6
>+	 */
>+	public void setUseContextToFilterImplicitImports(boolean useContextToFilterImplicitImports) {
>+		this.useContextToFilterImplicitImports = useContextToFilterImplicitImports;
>+	}
>+	
> 	private static int compareImport(char prefix, String qualifier, String name, String curr) {
> 		if (curr.charAt(0) != prefix || !curr.endsWith(name)) {
> 			return ImportRewriteContext.RES_NAME_UNKNOWN;
>@@ -360,9 +390,31 @@
> 				}
> 			}
> 		}
>+		if (this.filterImplicitImports && this.useContextToFilterImplicitImports) {
>+			String fPackageName= this.compilationUnit.getParent().getElementName();
>+			String mainTypeSimpleName= JavaCore.removeJavaLikeExtension(this.compilationUnit.getElementName());
>+			String fMainTypeName= concatenateName(fPackageName, mainTypeSimpleName);
>+			if (kind == ImportRewriteContext.KIND_TYPE
>+					&& (qualifier.equals(fPackageName)
>+							|| fMainTypeName.equals(concatenateName(qualifier, name))))
>+				return ImportRewriteContext.RES_NAME_FOUND;
>+		}
> 		return ImportRewriteContext.RES_NAME_UNKNOWN;
> 	}
> 
>+	private static String concatenateName(String name1, String name2) {
>+		StringBuffer buf= new StringBuffer();
>+		if (name1 != null && name1.length() > 0) {
>+			buf.append(name1);
>+		}
>+		if (name2 != null && name2.length() > 0) {
>+			if (buf.length() > 0) {
>+				buf.append('.');
>+			}
>+			buf.append(name2);
>+		}
>+		return buf.toString();
>+	}
> 	/**
> 	 * Adds a new import to the rewriter's record and returns a {@link Type} node that can be used
> 	 * in the code as a reference to the type. The type binding can be an array binding, type variable or wildcard.
>@@ -995,6 +1047,7 @@
> 
> 			ImportRewriteAnalyzer computer= new ImportRewriteAnalyzer(this.compilationUnit, usedAstRoot, this.importOrder, this.importOnDemandThreshold, this.staticImportOnDemandThreshold, this.restoreExistingImports);
> 			computer.setFilterImplicitImports(this.filterImplicitImports);
>+			computer.setUseContextToFilterImplicitImports(this.useContextToFilterImplicitImports);
> 
> 			if (this.addedImports != null) {
> 				for (int i= 0; i < this.addedImports.size(); i++) {
>Index: dom/org/eclipse/jdt/internal/core/dom/rewrite/ImportRewriteAnalyzer.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.core/dom/org/eclipse/jdt/internal/core/dom/rewrite/ImportRewriteAnalyzer.java,v
>retrieving revision 1.21
>diff -u -r1.21 ImportRewriteAnalyzer.java
>--- dom/org/eclipse/jdt/internal/core/dom/rewrite/ImportRewriteAnalyzer.java	7 Jan 2010 20:18:49 -0000	1.21
>+++ dom/org/eclipse/jdt/internal/core/dom/rewrite/ImportRewriteAnalyzer.java	10 Feb 2010 19:03:28 -0000
>@@ -24,6 +24,7 @@
> import org.eclipse.jdt.core.JavaCore;
> import org.eclipse.jdt.core.JavaModelException;
> import org.eclipse.jdt.core.Signature;
>+import org.eclipse.jdt.core.compiler.CharOperation;
> import org.eclipse.jdt.core.dom.ASTNode;
> import org.eclipse.jdt.core.dom.CompilationUnit;
> import org.eclipse.jdt.core.dom.ImportDeclaration;
>@@ -33,6 +34,7 @@
> import org.eclipse.jdt.core.search.IJavaSearchScope;
> import org.eclipse.jdt.core.search.SearchEngine;
> import org.eclipse.jdt.core.search.TypeNameRequestor;
>+import org.eclipse.jdt.internal.core.JavaProject;
> import org.eclipse.jface.text.IRegion;
> import org.eclipse.jface.text.Region;
> import org.eclipse.text.edits.DeleteEdit;
>@@ -53,6 +55,7 @@
> 	private final int staticImportOnDemandThreshold;
> 
> 	private boolean filterImplicitImports;
>+	private boolean useContextToFilterImplicitImports;
> 	private boolean findAmbiguousImports;
> 
> 	private int flags= 0;
>@@ -68,6 +71,7 @@
> 		this.staticImportOnDemandThreshold= staticThreshold;
> 
> 		this.filterImplicitImports= true;
>+		this.useContextToFilterImplicitImports= false;
> 		this.findAmbiguousImports= true; //!restoreExistingImports;
> 
> 		this.packageEntries= new ArrayList(20);
>@@ -168,9 +172,55 @@
> 		}
> 	}
> 
>-	private static String getQualifier(ImportDeclaration decl) {
>-		String name= decl.getName().getFullyQualifiedName();
>-		return decl.isOnDemand() ? name : Signature.getQualifier(name);
>+	private String getQualifier(ImportDeclaration decl) {
>+		String name = decl.getName().getFullyQualifiedName();
>+		/*
>+		 * If it's on demand import, return the fully qualified name. (e.g. pack1.Foo.* => pack.Foo, pack1.* => pack1)
>+		 * This is because we need to have pack1.Foo.* and pack1.Bar under different qualifier groups.
>+		 */
>+		if (decl.isOnDemand()) {
>+			return name;
>+		}
>+		return getQualifier(name, decl.isStatic());
>+	}
>+
>+	private String getQualifier(String name, boolean isStatic) {
>+		// For static imports, return the Type name as well as part of the qualifier
>+		if (isStatic || !this.useContextToFilterImplicitImports) {
>+			return Signature.getQualifier(name);
>+		}
>+
>+		char[] searchedName = name.toCharArray();
>+		int index = name.length();
>+		/* Stop at the last fragment */
>+		JavaProject project = (JavaProject) this.compilationUnit.getJavaProject();
>+		do {
>+			String testedName = new String(searchedName, 0, index);
>+			IJavaElement fragment = null;
>+			try {
>+				fragment = project.findPackageFragment(testedName);
>+			} catch (JavaModelException e) {
>+				return name;
>+			}
>+			if (fragment != null) {
>+				return testedName;
>+			}
>+			try {
>+				fragment = project.findType(testedName);
>+			} catch (JavaModelException e) {
>+				return name;
>+			}
>+			if (fragment != null) {
>+				index = CharOperation.lastIndexOf(Signature.C_DOT, searchedName, 0, index - 1);
>+			} else {
>+				// we use the heuristic that a name starting with a lowercase is a package name
>+				index = CharOperation.lastIndexOf(Signature.C_DOT, searchedName, 0, index - 1);
>+				if (Character.isLowerCase(searchedName[index + 1])) {
>+					return testedName;
>+				}
>+			}
>+		} while (index >= 0); 
>+		return name;
> 	}
> 
> 	private static String getFullName(ImportDeclaration decl) {
>@@ -238,18 +288,42 @@
> 	}
> 
> 	/**
>-	 * Sets that implicit imports (types in default package, CU- package and
>-	 * 'java.lang') should not be created. Note that this is a heuristic filter and can
>-	 * lead to missing imports, e.g. in cases where a type is forced to be specified
>-	 * due to a name conflict.
>-	 * By default, the filter is enabled.
>-	 * @param filterImplicitImports The filterImplicitImports to set
>+	 * Specifies that implicit imports (for types in <code>java.lang</code>, types in the same package as the rewrite
>+	 * compilation unit and types in the compilation unit's main type) should not be created, except if necessary to
>+	 * resolve an on-demand import conflict.
>+	 * 
>+	 * The filter is enabled by default.
>+	 * 
>+	 * 
>+	 * Note: {@link #setUseContextToFilterImplicitImports(boolean)} can be used to filter implicit imports
>+	 * when a context is used.
>+	 * 
>+	 * 
>+	 * @param filterImplicitImports
>+	 * if <code>true</code>, implicit imports will be filtered
>+	 * 
>+	 * @see #setUseContextToFilterImplicitImports(boolean)
> 	 */
> 	public void setFilterImplicitImports(boolean filterImplicitImports) {
> 		this.filterImplicitImports= filterImplicitImports;
> 	}
> 
> 	/**
>+	 * Sets whether a context should be used to properly filter implicit imports.
>+	 * 
>+	 * By default, the option is disabled.
>+	 * 
>+	 *
>+	 * @param useContextToFilterImplicitImports the given setting
>+	 * 
>+	 * @see #setFilterImplicitImports(boolean)
>+	 * @since 3.6
>+	 */
>+	public void setUseContextToFilterImplicitImports(boolean useContextToFilterImplicitImports) {
>+		this.useContextToFilterImplicitImports = useContextToFilterImplicitImports;
>+	}
>+
>+	/**
> 	 * When set searches for imports that can not be folded into on-demand
> 	 * imports but must be specified explicitly
> 	 * @param findAmbiguousImports The new value
>@@ -385,10 +459,11 @@
> 		return bestMatch;
> 	}
> 
>-	private static boolean isImplicitImport(String qualifier, ICompilationUnit cu) {
>+	private boolean isImplicitImport(String qualifier) {
> 		if (JAVA_LANG.equals(qualifier)) {
> 			return true;
> 		}
>+		ICompilationUnit cu= this.compilationUnit;
> 		String packageName= cu.getParent().getElementName();
> 		if (qualifier.equals(packageName)) {
> 			return true;
>@@ -401,13 +476,13 @@
> 	}
> 
> 	public void addImport(String fullTypeName, boolean isStatic) {
>-		String typeContainerName= Signature.getQualifier(fullTypeName);
>+		String typeContainerName= getQualifier(fullTypeName, isStatic);
> 		ImportDeclEntry decl= new ImportDeclEntry(fullTypeName, isStatic, null);
> 		sortIn(typeContainerName, decl, isStatic);
> 	}
> 
> 	public boolean removeImport(String qualifiedName, boolean isStatic) {
>-		String containerName= Signature.getQualifier(qualifiedName);
>+		String containerName= getQualifier(qualifiedName, isStatic);
> 
> 		int nPackages= this.packageEntries.size();
> 		for (int i= 0; i < nPackages; i++) {
>@@ -453,9 +528,11 @@
> 				PackageEntry packEntry= new PackageEntry(typeContainerName, group, isStatic);
> 				packEntry.add(decl);
> 				int index= this.packageEntries.indexOf(bestMatch);
>-				if (cmp < 0) { 	// insert ahead of best match
>+				if (cmp < 0) {
>+					// insert ahead of best match
> 					this.packageEntries.add(index, packEntry);
>-				} else {		// insert after best match
>+				} else {
>+					// insert after best match
> 					this.packageEntries.add(index + 1, packEntry);
> 				}
> 			}
>@@ -522,17 +599,14 @@
> 			int nPackageEntries= this.packageEntries.size();
> 			for (int i= 0; i < nPackageEntries; i++) {
> 				PackageEntry pack= (PackageEntry) this.packageEntries.get(i);
>-				int nImports= pack.getNumberOfImports();
>-
>-				if (this.filterImplicitImports && !pack.isStatic() && isImplicitImport(pack.getName(), this.compilationUnit)) {
>-					pack.removeAllNew(onDemandConflicts);
>-					nImports= pack.getNumberOfImports();
>+				if (this.filterImplicitImports && !pack.isStatic() && isImplicitImport(pack.getName())) {
>+					pack.filterImplicitImports(this.useContextToFilterImplicitImports);
> 				}
>+				int nImports= pack.getNumberOfImports();
> 				if (nImports == 0) {
> 					continue;
> 				}
> 
>-
> 				if (spacesBetweenGroups > 0) {
> 					// add a space between two different groups by looking at the two adjacent imports
> 					if (lastPackage != null && !pack.isComment() && !pack.isSameGroup(lastPackage)) {
>@@ -928,12 +1002,21 @@
> 			return false;
> 		}
> 
>-		public void removeAllNew(Set onDemandConflicts) {
>+		public void filterImplicitImports(boolean useContextToFilterImplicitImports) {
> 			int nInports= this.importEntries.size();
> 			for (int i= nInports - 1; i >= 0; i--) {
> 				ImportDeclEntry curr= getImportAt(i);
>-				if (curr.isNew() /*&& (onDemandConflicts == null || onDemandConflicts.contains(curr.getSimpleName()))*/) {
>-					this.importEntries.remove(i);
>+				if (curr.isNew()) {
>+					if (!useContextToFilterImplicitImports) {
>+						this.importEntries.remove(i);
>+					} else {
>+						String elementName = curr.getElementName();
>+						int lastIndexOf = elementName.lastIndexOf('.');
>+						boolean internalClassImport = lastIndexOf > getName().length();
>+						if (!internalClassImport) {
>+							this.importEntries.remove(i);
>+						}
>+					}
> 				}
> 			}
> 		}
>#P org.eclipse.jdt.core.tests.model
>Index: src/org/eclipse/jdt/core/tests/rewrite/describing/ImportRewriteTest.java
>===================================================================
>RCS file: /cvsroot/eclipse/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/rewrite/describing/ImportRewriteTest.java,v
>retrieving revision 1.15
>diff -u -r1.15 ImportRewriteTest.java
>--- src/org/eclipse/jdt/core/tests/rewrite/describing/ImportRewriteTest.java	21 Jan 2010 16:46:36 -0000	1.15
>+++ src/org/eclipse/jdt/core/tests/rewrite/describing/ImportRewriteTest.java	10 Feb 2010 19:03:29 -0000
>@@ -572,6 +572,145 @@
> 		assertEqualString(cu.getSource(), buf.toString());
> 	}
> 
>+	/**
>+	 * Test that the Inner class import comes in the right order (i.e. after the enclosing type's import) when re-organized
>+	 * 
>+	 * @see "https://bugs.eclipse.org/bugs/show_bug.cgi?id=194358"
>+	 */
>+	public void testBug194358() throws Exception {
>+
>+		StringBuffer buf= new StringBuffer();
>+		buf.append("package pack1;\n");
>+		buf.append("\n");
>+		buf.append("import pack2.A;\n");
>+		buf.append("import pack2.A.Inner;\n");
>+		buf.append("import pack2.B;\n");
>+		buf.append("\n");
>+		buf.append("public class C {\n");
>+		buf.append("}\n");
>+
>+		IPackageFragment pack1= this.sourceFolder.createPackageFragment("pack1", false, null);
>+		ICompilationUnit cu= pack1.createCompilationUnit("C.java", buf.toString(), false, null);
>+
>+		// We need to actually make some state in the AST for the classes, to test that we can 
>+		// disambiguate between packages and inner classes (see the bug for details).
>+		IPackageFragment pack2= this.sourceFolder.createPackageFragment("pack2", false, null);
>+		ICompilationUnit aUnit= pack2.createCompilationUnit("A.java", "", false, null);
>+		ICompilationUnit bUnit= pack2.createCompilationUnit("B.java", "", false, null);
>+		bUnit.createType("class B {}", null, false, null);
>+
>+		IType aType= aUnit.createType("class A {}", null, false, null);
>+		aType.createType("class Inner {}", null, false, null);
>+		String[] order= new String[] { "java" };
>+
>+		ImportRewrite imports= newImportsRewrite(cu, order, 99, 99, false);
>+		imports.setUseContextToFilterImplicitImports(false);
>+		imports.addImport("pack2.A");
>+		imports.addImport("pack2.B");
>+		imports.addImport("pack2.A.Inner");
>+
>+		apply(imports);
>+
>+		buf= new StringBuffer();
>+		buf.append("package pack1;\n");
>+		buf.append("\n");
>+		buf.append("import pack2.A;\n");
>+		buf.append("import pack2.A.Inner;\n");
>+		buf.append("import pack2.B;\n");
>+		buf.append("\n");
>+		buf.append("public class C {\n");
>+		buf.append("}\n");
>+		assertEqualString(cu.getSource(), buf.toString());
>+	}
>+
>+	/**
>+	 * Test that a valid inner class import is not removed even when the container
>+	 * class is implicitly available. This tests the case where the classes are in 
>+	 * different compilation units.
>+	 * 
>+	 * @see "https://bugs.eclipse.org/bugs/show_bug.cgi?id=194358"
>+	 */
>+	public void testBug194358a() throws Exception {
>+		StringBuffer buf= new StringBuffer();
>+		buf.append("package com.pack1;\n");
>+		buf.append("\n");
>+		buf.append("import com.pack1.A;\n");
>+		buf.append("import com.pack1.A.Inner;\n");
>+		buf.append("import com.pack2.B;\n");
>+		buf.append("\n");
>+		buf.append("public class C {\n");
>+		buf.append("}\n");
>+
>+		IPackageFragment pack1= this.sourceFolder.createPackageFragment("com.pack1", false, null);
>+		ICompilationUnit cu= pack1.createCompilationUnit("C.java", buf.toString(), false, null);
>+		ICompilationUnit aUnit= pack1.createCompilationUnit("A.java", "", false, null);
>+
>+		IPackageFragment pack2= this.sourceFolder.createPackageFragment("com.pack2", false, null);
>+		ICompilationUnit bUnit= pack2.createCompilationUnit("B.java", "", false, null);
>+		bUnit.createType("class B {}", null, false, null);
>+		IType aType= aUnit.createType("class A {}", null, false, null);
>+		aType.createType("class Inner {}", null, false, null);
>+		String[] order= new String[] { "java" };
>+
>+		ImportRewrite imports= newImportsRewrite(cu, order, 99, 99, false);
>+		imports.setUseContextToFilterImplicitImports(false);
>+		imports.addImport("com.pack1.A");
>+		imports.addImport("com.pack1.A.Inner");
>+		imports.addImport("com.pack2.B");
>+
>+		apply(imports);
>+
>+		buf= new StringBuffer();
>+		buf.append("package com.pack1;\n");
>+		buf.append("\n");
>+		buf.append("import com.pack1.A.Inner;\n");
>+		buf.append("import com.pack2.B;\n");
>+		buf.append("\n");
>+		buf.append("public class C {\n");
>+		buf.append("}\n");
>+		assertEqualString(cu.getSource(), buf.toString());
>+	}
>+	/**
>+	 * Test that the Inner type imports are not removed while organizing even though the 
>+	 * containing class is implicitly available - for the case when both the classes are 
>+	 * in the same compilation unit
>+	 * 
>+	 * see "https://bugs.eclipse.org/bugs/show_bug.cgi?id=235253"
>+	 */
>+	public void testBug235253() throws Exception {
>+		StringBuffer buf= new StringBuffer();
>+		buf.append("package bug;\n");
>+		buf.append("\n");
>+		buf.append("class Bug {\n");
>+		buf.append("public void addFile(File file) {}\n");
>+		buf.append("\tinterface Proto{};\n");
>+		buf.append("}\n");		
>+		buf.append("class Foo implements Proto{}");
>+
>+		IPackageFragment pack1= this.sourceFolder.createPackageFragment("bug", false, null);
>+		ICompilationUnit cu= pack1.createCompilationUnit("Bug.java", buf.toString(), false, null);
>+		String[] order= new String[] { "bug" , "java" };
>+		ImportRewrite imports= newImportsRewrite(cu, order, 99, 99, false);
>+		imports.setUseContextToFilterImplicitImports(false);
>+		imports.addImport("bug.Bug.Proto");
>+		imports.addImport("java.io.File"); 
>+		
>+		apply(imports);
>+		buf = new StringBuffer();
>+		buf.append("package bug;\n");
>+		buf.append("\n");
>+		buf.append("import bug.Bug.Proto;\n");
>+		buf.append("\n");
>+		buf.append("import java.io.File;\n");
>+		buf.append("\n");
>+		buf.append("class Bug {\n");
>+		buf.append("public void addFile(File file) {}\n");
>+		buf.append("\tinterface Proto{};\n");
>+		buf.append("}\n");		
>+		buf.append("class Foo implements Proto{}");
>+		assertEqualString(cu.getSource(), buf.toString());
>+	}
>+		
> 	public void testAddStaticImports1() throws Exception {
> 
> 		IPackageFragment pack1= this.sourceFolder.createPackageFragment("pack1", false, null);

Actions: View | Diff

Attachments on bug 235253: 151581 | 152493 | 157791 | 158758 | 158804