Fixed: [ Issue 60 ] Imported types do not need to be fully-qualified https://code.google.com/p/protobuf-dt/issues/detail?id=60 Package resolution is done correctly now.
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider_localNames_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider_namesOf_Test.java similarity index 92% rename from com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider_localNames_Test.java rename to com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider_namesOf_Test.java index 2a32898..55e1202 100644 --- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider_localNames_Test.java +++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider_namesOf_Test.java
@@ -23,11 +23,11 @@ import com.google.eclipse.protobuf.protobuf.Enum; /** - * Tests for <code>{@link LocalNamesProvider#localNames(EObject)}</code>. + * Tests for <code>{@link LocalNamesProvider#namesOf(EObject)}</code>. * * @author alruiz@google.com (Alex Ruiz) */ -public class LocalNamesProvider_localNames_Test { +public class LocalNamesProvider_namesOf_Test { @Rule public XtextRule xtext = new XtextRule(); @@ -53,7 +53,7 @@ proto.append("} "); Protobuf root = xtext.parse(proto); Enum phoneType = findEnum("PhoneType", root); - List<QualifiedName> names = namesProvider.localNames(phoneType); + List<QualifiedName> names = namesProvider.namesOf(phoneType); assertThat(names.get(0).toString(), equalTo("PhoneType")); assertThat(names.get(1).toString(), equalTo("PhoneNumber.PhoneType")); assertThat(names.get(2).toString(), equalTo("Person.PhoneNumber.PhoneType"));
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/PackageResolver_areRelated_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/PackageResolver_areRelated_Test.java index 23a7935..284a1cf 100644 --- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/PackageResolver_areRelated_Test.java +++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/scoping/PackageResolver_areRelated_Test.java
@@ -29,11 +29,21 @@ resolver = new PackageResolver(); } + private String baseName; + private String[] subpackageNames; private Package p1; private Package p2; @Before public void setUp() { - p1 = new PackageStub("may.the.force.be.with.you"); + baseName = "may.the.force.be.with.you"; + subpackageNames = new String[] { + "may.the.force.be.with", + "may.the.force.be.", + "may.the.force.", + "may.the", + "may" + }; + p1 = new PackageStub(baseName); p2 = new PackageStub(); } @@ -41,4 +51,29 @@ p2.setName(p1.getName()); assertThat(resolver.areRelated(p1, p2), equalTo(true)); } + + @Test public void should_return_true_second_is_subPackage_of_first() { + for (String name : subpackageNames) { + p2.setName(name); + assertThat(resolver.areRelated(p1, p2), equalTo(true)); + } + } + + @Test public void should_return_true_first_is_subPackage_of_second() { + p2.setName(baseName); + for (String name : subpackageNames) { + p1.setName(name); + assertThat(resolver.areRelated(p1, p2), equalTo(true)); + } + } + + @Test public void should_return_false_if_second_starts_with_few_segments_of_first_but_is_not_subpackage() { + p2.setName("may.the.ring"); + assertThat(resolver.areRelated(p1, p2), equalTo(false)); + } + + @Test public void should_return_false_if_names_are_completely_different() { + p2.setName("peace.dog"); + assertThat(resolver.areRelated(p1, p2), equalTo(false)); + } }
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider.java index 5d4f98a..98ccb0b 100644 --- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider.java +++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/LocalNamesProvider.java
@@ -57,50 +57,53 @@ */ class LocalNamesProvider { - @Inject private final IQualifiedNameConverter converter = new IQualifiedNameConverter.DefaultImpl(); @Inject private final IResourceScopeCache cache = IResourceScopeCache.NullImpl.INSTANCE; + @Inject private final IQualifiedNameConverter converter = new IQualifiedNameConverter.DefaultImpl(); @Inject private ProtobufElementFinder finder; private final Function<EObject, String> resolver = newResolver(String.class, "name"); - List<QualifiedName> localNames(final EObject obj) { + List<QualifiedName> namesOf(final EObject obj) { Pair<EObject, String> key = pair(obj, "fqns"); return cache.get(key, obj.eResource(), new Provider<List<QualifiedName>>() { public List<QualifiedName> get() { - List<QualifiedName> names = new ArrayList<QualifiedName>(); + List<QualifiedName> allNames = new ArrayList<QualifiedName>(); EObject current = obj; String name = resolver.apply(current); - if (isEmpty(name)) return names; + if (isEmpty(name)) return emptyList(); QualifiedName qualifiedName = converter.toQualifiedName(name); - names.add(qualifiedName); + allNames.add(qualifiedName); while (current.eContainer() != null) { current = current.eContainer(); String containerName = resolver.apply(current); if (isEmpty(containerName)) continue; qualifiedName = converter.toQualifiedName(containerName).append(qualifiedName); - names.add(qualifiedName); + allNames.add(qualifiedName); } - names.addAll(addPackageSegments(qualifiedName)); - return unmodifiableList(names); - } - - private List<QualifiedName> addPackageSegments(QualifiedName qualifiedName) { - Package p = finder.packageOf(obj); - if (p == null) return emptyList(); - String packageName = p.getName(); - if (isEmpty(packageName)) return emptyList(); - QualifiedName name = qualifiedName; - List<String> segments = converter.toQualifiedName(packageName).getSegments(); - int segmentCount = segments.size(); - if (segmentCount == 1) return emptyList(); - List<QualifiedName> names = new ArrayList<QualifiedName>(); - for (int i = segmentCount - 1; i > 0; i--) { - name = QualifiedName.create(segments.get(i)).append(name); - names.add(name); - } - return unmodifiableList(names); + allNames.addAll(addPackageNameSegments(qualifiedName, finder.packageOf(obj))); + return unmodifiableList(allNames); } }); } + + private List<QualifiedName> addPackageNameSegments(QualifiedName qualifiedName, Package p) { + QualifiedName name = qualifiedName; + List<String> segments = fqnSegments(p); + int segmentCount = segments.size(); + if (segmentCount <= 1) return emptyList(); + List<QualifiedName> allNames = new ArrayList<QualifiedName>(); + for (int i = segmentCount - 1; i > 0; i--) { + name = QualifiedName.create(segments.get(i)).append(name); + allNames.add(name); + } + return unmodifiableList(allNames); + } + + private List<String> fqnSegments(Package p) { + if (p == null) return emptyList(); + String packageName = p.getName(); + if (isEmpty(packageName)) return emptyList(); + return converter.toQualifiedName(packageName).getSegments(); + } }
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/PackageResolver.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/PackageResolver.java index 16b21b8..1044fd6 100644 --- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/PackageResolver.java +++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/PackageResolver.java
@@ -10,15 +10,16 @@ import static org.eclipse.xtext.util.Strings.isEmpty; +import java.util.List; + import org.eclipse.xtext.naming.*; import com.google.eclipse.protobuf.protobuf.Package; -import com.google.inject.*; +import com.google.inject.Inject; /** * @author alruiz@google.com (Alex Ruiz) */ -@Singleton class PackageResolver { @Inject private final IQualifiedNameConverter converter = new IQualifiedNameConverter.DefaultImpl(); @@ -29,7 +30,7 @@ QualifiedName name2 = nameOf(p2); if (name1 == null || name2 == null) return false; if (name1.equals(name2)) return true; - return false; + return (isSubPackage(name1, name2)); } private QualifiedName nameOf(Package p) { @@ -37,4 +38,15 @@ if (isEmpty(name)) return null; return converter.toQualifiedName(name); } + + private boolean isSubPackage(QualifiedName name1, QualifiedName name2) { + List<String> segments2 = name2.getSegments(); + int segment2Count = segments2.size(); + int counter = 0; + for (String segment1 : name1.getSegments()) { + if (!segment1.equals(segments2.get(counter++))) return false; + if (counter == segment2Count) break; + } + return true; + } }
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/ProtobufScopeProvider.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/ProtobufScopeProvider.java index 1f9b150..9758fe2 100644 --- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/ProtobufScopeProvider.java +++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/scoping/ProtobufScopeProvider.java
@@ -8,6 +8,7 @@ */ package com.google.eclipse.protobuf.scoping; +import static java.util.Collections.emptyList; import static org.eclipse.emf.common.util.URI.createURI; import static org.eclipse.emf.ecore.util.EcoreUtil.getAllContents; import static org.eclipse.xtext.resource.EObjectDescription.create; @@ -16,14 +17,15 @@ import org.eclipse.emf.common.util.*; import org.eclipse.emf.ecore.*; -import org.eclipse.emf.ecore.resource.*; +import org.eclipse.emf.ecore.resource.ResourceSet; import org.eclipse.xtext.naming.*; -import org.eclipse.xtext.resource.IEObjectDescription; +import org.eclipse.xtext.resource.*; import org.eclipse.xtext.scoping.IScope; import org.eclipse.xtext.scoping.impl.*; import com.google.eclipse.protobuf.protobuf.*; import com.google.eclipse.protobuf.protobuf.Enum; +import com.google.eclipse.protobuf.protobuf.Package; import com.google.eclipse.protobuf.util.ProtobufElementFinder; import com.google.inject.Inject; @@ -43,6 +45,7 @@ @Inject private IQualifiedNameProvider nameProvider; @Inject private ImportUriResolver uriResolver; @Inject private LocalNamesProvider localNamesProvider; + @Inject private PackageResolver packageResolver; @SuppressWarnings("unused") IScope scope_TypeReference_type(TypeReference typeRef, EReference reference) { @@ -82,7 +85,7 @@ List<IEObjectDescription> descriptions = new ArrayList<IEObjectDescription>(); for (EObject element : root.eContents()) { if (!targetType.isInstance(element)) continue; - List<QualifiedName> names = localNamesProvider.localNames(element); + List<QualifiedName> names = localNamesProvider.namesOf(element); int nameCount = names.size(); for (int i = level; i < nameCount; i++) { descriptions.add(create(names.get(i), element)); @@ -97,17 +100,34 @@ } private <T extends Type> Collection<IEObjectDescription> importedTypes(Protobuf root, Class<T> targetType) { - ResourceSet resourceSet = root.eResource().getResourceSet(); + List<Import> imports = finder.importsIn(root); + if (imports.isEmpty()) return emptyList(); + Package importRootPackage = finder.packageOf(root); List<IEObjectDescription> descriptions = new ArrayList<IEObjectDescription>(); - for (Import anImport : finder.importsIn(root)) { - URI importUri = createURI(uriResolver.apply(anImport)); - Resource imported = resourceSet.getResource(importUri, true); - descriptions.addAll(innerTypes(imported, targetType)); + for (Import anImport : imports) { + XtextResource imported = importedResource(anImport); + EObject importedRoot = imported.getParseResult().getRootASTElement(); + if (arePackagesRelated(importRootPackage, importedRoot)) { + descriptions.addAll(typesIn(importedRoot)); + continue; + } + descriptions.addAll(children(imported, targetType)); } return descriptions; } - private <T extends Type> Collection<IEObjectDescription> innerTypes(Resource resource, Class<T> targetType) { + private XtextResource importedResource(Import anImport) { + ResourceSet resourceSet = finder.rootOf(anImport).eResource().getResourceSet(); + URI importUri = createURI(uriResolver.apply(anImport)); + return (XtextResource) resourceSet.getResource(importUri, true); + } + + private boolean arePackagesRelated(Package aPackage, EObject root) { + Package p = finder.packageOf(root); + return packageResolver.areRelated(aPackage, p); + } + + private <T extends Type> Collection<IEObjectDescription> children(XtextResource resource, Class<T> targetType) { List<IEObjectDescription> descriptions = new ArrayList<IEObjectDescription>(); TreeIterator<Object> contents = getAllContents(resource, true); while (contents.hasNext()) {