Bug 514152 - additional component annotation validations

- servicefactory w.r.t. services, immediate, and factory
- immediate w.r.t. services and factory
- scope w.r.t. factory and immediate
- duplicate configurationPids

Change-Id: Iab988314191843985af2a62db6925ff06bdf8dd1
Signed-off-by: Peter Nehrer <pnehrer@eclipticalsoftware.com>
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponent.java
index 28166d1..1cf3de5 100644
--- a/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponent.java
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponent.java
@@ -25,7 +25,7 @@
 		configurationPolicy = ConfigurationPolicy.REQUIRE,
 		enabled = false,
 		factory = "test.componentFactory",
-		immediate = true,
+		immediate = false,
 		name = "test.fullComponent",
 		properties = {
 				"/fullComponent1.properties",
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponentV1_2.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponentV1_2.java
index 41480a0..f88b626 100644
--- a/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponentV1_2.java
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test1/src/ds/annotations/test1/FullComponentV1_2.java
@@ -21,7 +21,7 @@
 		configurationPolicy = ConfigurationPolicy.REQUIRE,
 		enabled = false,
 		factory = "test.componentFactory",
-		immediate = true,
+		immediate = false,
 		name = "test.fullComponent-v1_2",
 		properties = {
 				"/fullComponent1.properties",
@@ -44,7 +44,7 @@
 				"explicitStringArrayProperty:String=explicitStringArrayValue3",
 		},
 		service = Map.class,
-		servicefactory = true)
+		servicefactory = false)
 public class FullComponentV1_2 extends AbstractMap<String, Object> {
 
 	private volatile Set<Map.Entry<String, Object>> entrySet = new HashSet<>();
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/DelayedWithNoServicesComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/DelayedWithNoServicesComponent.java
new file mode 100644
index 0000000..562deb0
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/DelayedWithNoServicesComponent.java
@@ -0,0 +1,8 @@
+package ds.annotations.test2;
+
+import org.osgi.service.component.annotations.Component;
+
+@Component(immediate = false)
+public class DelayedWithNoServicesComponent {
+
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/DuplicateConfigurationPidComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/DuplicateConfigurationPidComponent.java
new file mode 100644
index 0000000..d4e430a
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/DuplicateConfigurationPidComponent.java
@@ -0,0 +1,8 @@
+package ds.annotations.test2;
+
+import org.osgi.service.component.annotations.Component;
+
+@Component(configurationPid = {"ds.annotations.test2.DuplicateConfigurationPidComponent", "foo", "$"})
+public class DuplicateConfigurationPidComponent {
+
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/FactoryImmediateComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/FactoryImmediateComponent.java
new file mode 100644
index 0000000..295cc3e
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/FactoryImmediateComponent.java
@@ -0,0 +1,8 @@
+package ds.annotations.test2;
+
+import org.osgi.service.component.annotations.Component;
+
+@Component(factory = "foo", immediate = true)
+public class FactoryImmediateComponent {
+
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/FactoryOrImmediateServiceFactoryComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/FactoryOrImmediateServiceFactoryComponent.java
new file mode 100644
index 0000000..be14916
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/FactoryOrImmediateServiceFactoryComponent.java
@@ -0,0 +1,19 @@
+package ds.annotations.test2;
+
+import java.util.EventListener;
+
+import org.osgi.service.component.annotations.Component;
+
+@Component(servicefactory = true, immediate = true)
+public class FactoryOrImmediateServiceFactoryComponent implements EventListener {
+
+	@Component(servicefactory = true, factory = "foo")
+	public static class FactoryServiceFactoryComponent implements EventListener {
+		
+	}
+
+	@Component(servicefactory = true)
+	public static class ThisIsOkComponent implements EventListener {
+		
+	}
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ErrorComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/MissingDynamicReferenceUnbindMethodComponent.java
similarity index 89%
rename from ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ErrorComponent.java
rename to ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/MissingDynamicReferenceUnbindMethodComponent.java
index 6de0250..9c0aaae 100644
--- a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ErrorComponent.java
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/MissingDynamicReferenceUnbindMethodComponent.java
@@ -8,7 +8,7 @@
 import org.osgi.service.component.annotations.ReferencePolicy;
 
 @Component
-public class ErrorComponent {
+public class MissingDynamicReferenceUnbindMethodComponent {
 
 	@Reference
 	void setStaticReference(Executor executor) {
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/NonSingletonFactoryOrImmediateComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/NonSingletonFactoryOrImmediateComponent.java
new file mode 100644
index 0000000..b0811c9
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/NonSingletonFactoryOrImmediateComponent.java
@@ -0,0 +1,15 @@
+package ds.annotations.test2;
+
+import java.util.EventListener;
+
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ServiceScope;
+
+@Component(scope = ServiceScope.BUNDLE, immediate = true)
+public class NonSingletonFactoryOrImmediateComponent implements EventListener {
+
+	@Component(scope = ServiceScope.PROTOTYPE, factory = "foo")
+	public static class NonSingletonFactoryComponent implements EventListener {
+		
+	}
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ScopeWithNoServicesComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ScopeWithNoServicesComponent.java
new file mode 100644
index 0000000..f45ed3e
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ScopeWithNoServicesComponent.java
@@ -0,0 +1,9 @@
+package ds.annotations.test2;
+
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ServiceScope;
+
+@Component(scope = ServiceScope.BUNDLE)
+public class ScopeNoServicesComponent {
+
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ServiceFactoryWithNoServicesComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ServiceFactoryWithNoServicesComponent.java
new file mode 100644
index 0000000..13b2baf
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ServiceFactoryWithNoServicesComponent.java
@@ -0,0 +1,8 @@
+package ds.annotations.test2;
+
+import org.osgi.service.component.annotations.Component;
+
+@Component(servicefactory = true)
+public class ServiceFactoryWithNoServicesComponent{
+
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ServiceFactoryWithScopeComponent.java b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ServiceFactoryWithScopeComponent.java
new file mode 100644
index 0000000..7429351
--- /dev/null
+++ b/ds/org.eclipse.pde.ds.annotations.tests/projects/test2/src/ds/annotations/test2/ServiceFactoryWithScopeComponent.java
@@ -0,0 +1,18 @@
+package ds.annotations.test2;
+
+import java.util.EventListener;
+
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.ServiceScope;
+
+@Component(servicefactory = false, scope = ServiceScope.BUNDLE)
+public class ServiceFactoryWithScopeComponent implements EventListener {
+
+	@Component(servicefactory = true, scope = ServiceScope.BUNDLE)
+	public static class ThisIsOkComponent implements EventListener {
+	}
+
+	@Component(servicefactory = false, scope = ServiceScope.SINGLETON)
+	public static class AlsoOkComponent implements EventListener {
+	}
+}
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/ErrorProjectTest.java b/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/ErrorProjectTest.java
index a2b4e9c..a7ea355 100644
--- a/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/ErrorProjectTest.java
+++ b/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/ErrorProjectTest.java
@@ -1,22 +1,106 @@
 package org.eclipse.pde.ds.internal.annotations.tests;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
+import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IMarker;
 import org.eclipse.core.resources.IResource;
+import org.eclipse.core.runtime.IPath;
+import org.eclipse.core.runtime.Path;
 import org.junit.Test;
 
 public class ErrorProjectTest extends CompilationParticipantTest {
 
+	private static final IPath PATH_PREFIX = new Path("src/ds/annotations/test2/");
+
 	@Override
 	protected String getTestProjectName() {
 		return "ds.annotations.test2";
 	}
 
+	private IFile getFixture(String simpleClassName) {
+		IFile file = testProject.getFile(PATH_PREFIX.append(simpleClassName).addFileExtension("java"));
+		assertTrue(file.exists());
+		return file;
+	}
+
 	@Test
 	public void missingImplicitDynamicReferenceUnbindMethodError() throws Exception {
-		IMarker[] markers = testProject.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_INFINITE);
+		IResource cu = getFixture("MissingDynamicReferenceUnbindMethodComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
 		assertEquals(1, markers.length);
 		assertEquals("No implicit unbind method named 'unsetDynamicReference' found in implementation class.", markers[0].getAttribute(IMarker.MESSAGE));
 	}
+
+	@Test
+	public void duplicateConfigurationPidError() throws Exception {
+		IResource cu = getFixture("DuplicateConfigurationPidComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(2, markers.length);
+		for (int i = 0; i < markers.length; ++i) {
+			assertEquals("Duplicate configuration PID.", markers[i].getAttribute(IMarker.MESSAGE));
+		}
+	}
+
+	@Test
+	public void factoryImmediateError() throws Exception {
+		IResource cu = getFixture("FactoryImmediateComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(1, markers.length);
+		assertEquals("Factory component cannot be immediate.", markers[0].getAttribute(IMarker.MESSAGE));
+	}
+
+	@Test
+	public void delayedWithNoServicesError() throws Exception {
+		IResource cu = getFixture("DelayedWithNoServicesComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(1, markers.length);
+		assertEquals("Component that does not register services must be immediate.", markers[0].getAttribute(IMarker.MESSAGE));
+	}
+
+	@Test
+	public void nonSingletonFactoryOrImmediateError() throws Exception {
+		IResource cu = getFixture("NonSingletonFactoryOrImmediateComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(2, markers.length);
+		for (int i = 0; i < markers.length; ++i) {
+			assertEquals("Factory or immediate component must have singleton scope.", markers[i].getAttribute(IMarker.MESSAGE));
+		}
+	}
+
+	@Test
+	public void scopeWithNoServicesError() throws Exception {
+		IResource cu = getFixture("ScopeWithNoServicesComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(1, markers.length);
+		assertEquals("Scope is not applicable to component with no registered services.", markers[0].getAttribute(IMarker.MESSAGE));
+	}
+
+
+	@Test
+	public void factoryOrImmediateServiceFactoryError() throws Exception {
+		IResource cu = getFixture("FactoryOrImmediateServiceFactoryComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(2, markers.length);
+		for (int i = 0; i < markers.length; ++i) {
+			assertEquals("Factory or immediate component cannot be a service factory.", markers[i].getAttribute(IMarker.MESSAGE));
+		}
+	}
+
+	@Test
+	public void serviceFactoryIgnoredError() throws Exception {
+		IResource cu = getFixture("ServiceFactoryWithScopeComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(1, markers.length);
+		assertEquals("Property 'servicefactory' is ignored when non-default scope is specified.", markers[0].getAttribute(IMarker.MESSAGE));
+	}
+
+	@Test
+	public void serviceFactoryWithNoServicesError() throws Exception {
+		IResource cu = getFixture("ServiceFactoryWithNoServicesComponent");
+		IMarker[] markers = cu.findMarkers(DS_PROBLEM_MARKER, true, IResource.DEPTH_ZERO);
+		assertEquals(1, markers.length);
+		assertEquals("Component that does not register services cannot be a service factory.", markers[0].getAttribute(IMarker.MESSAGE));
+	}
 }
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTest.java b/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTest.java
index eced28b..e5b5afd 100644
--- a/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTest.java
+++ b/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTest.java
@@ -4,7 +4,6 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeNotNull;
 import static org.junit.Assume.assumeTrue;
 
@@ -60,7 +59,7 @@
 
 	@Test
 	public void componentImmediate() throws Exception {
-		assertTrue(dsModel.getDSComponent().getImmediate());
+		assertFalse(dsModel.getDSComponent().getImmediate());
 	}
 
 	@Test
diff --git a/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTestV1_2.java b/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTestV1_2.java
index 5ea248a..caa4130 100644
--- a/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTestV1_2.java
+++ b/ds/org.eclipse.pde.ds.annotations.tests/src/org/eclipse/pde/ds/internal/annotations/tests/FullComponentTestV1_2.java
@@ -4,7 +4,6 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeNotNull;
 import static org.junit.Assume.assumeTrue;
 
@@ -60,7 +59,7 @@
 
 	@Test
 	public void componentImmediate() throws Exception {
-		assertTrue(dsModel.getDSComponent().getImmediate());
+		assertFalse(dsModel.getDSComponent().getImmediate());
 	}
 
 	@Test
@@ -94,7 +93,7 @@
 	public void componentServiceProviderInterface() throws Exception {
 		IDSService service = dsModel.getDSComponent().getService();
 		assertNotNull(service);
-		assertTrue(service.getServiceFactory());
+		assertFalse(service.getServiceFactory());
 		IDSProvide[] provides = service.getProvidedServices();
 		assertNotNull(provides);
 		assertEquals(1, provides.length);
diff --git a/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/AnnotationVisitor.java b/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/AnnotationVisitor.java
index 56e066a..665225d 100644
--- a/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/AnnotationVisitor.java
+++ b/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/AnnotationVisitor.java
@@ -125,6 +125,10 @@
 
 	private static final String VALUE_SERVICE_SCOPE_DEFAULT = DSEnums.getServiceScope("DEFAULT"); //$NON-NLS-1$
 
+	private static final String VALUE_SERVICE_SCOPE_SINGLETON = DSEnums.getServiceScope("SINGLETON"); //$NON-NLS-1$
+
+	private static final String VALUE_SERVICE_SCOPE_BUNDLE = DSEnums.getServiceScope("BUNDLE"); //$NON-NLS-1$
+
 	private static final Set<String> PROPERTY_TYPES = Collections.unmodifiableSet(
 			new HashSet<>(
 					Arrays.asList(
@@ -220,7 +224,7 @@
 					|| (isNested = (!type.isPackageMemberTypeDeclaration() && !isNestedPublicStatic(type)))
 					|| (noDefaultConstructor = !hasDefaultConstructor(type))) {
 				// interfaces, abstract types, non-static/non-public nested types, or types with no default constructor cannot be components
-				if (errorLevel != ValidationErrorLevel.ignore) {
+				if (!errorLevel.isIgnore()) {
 					if (isInterface) {
 						problemReporter.reportProblem(annotation, null, NLS.bind(Messages.AnnotationProcessor_invalidCompImplClass_interface, type.getName().getIdentifier()), type.getName().getIdentifier());
 					} else if (isAbstract) {
@@ -498,19 +502,20 @@
 			for (int i = 0; i < elements.length; ++i) {
 				ITypeBinding serviceType = (ITypeBinding) elements[i];
 				String serviceName = serviceType.getBinaryName();
-				if (!errorLevel.isIgnore()) {
-					if (serviceDuplicates.containsKey(serviceName)) {
+				if (services.add(serviceName)) {
+					if (serviceDuplicates != null) {
+						serviceDuplicates.put(serviceName, i);
+					}
+				} else {
+					if (serviceDuplicates != null) {
 						problemReporter.reportProblem(annotation, "service", i, Messages.AnnotationProcessor_duplicateServiceDeclaration, serviceName); //$NON-NLS-1$
 						Integer pos = serviceDuplicates.put(serviceName, null);
 						if (pos != null) {
 							problemReporter.reportProblem(annotation, "service", pos.intValue(), Messages.AnnotationProcessor_duplicateServiceDeclaration, serviceName); //$NON-NLS-1$
 						}
-					} else {
-						serviceDuplicates.put(serviceName, i);
 					}
 				}
 
-				services.add(serviceName);
 				validateComponentService(annotation, typeBinding, serviceType, i);
 			}
 		} else {
@@ -530,6 +535,9 @@
 		Boolean serviceFactory = null;
 		if ((value = params.get("servicefactory")) instanceof Boolean) { //$NON-NLS-1$
 			serviceFactory = (Boolean) value;
+			if (!errorLevel.isIgnore() && Boolean.TRUE.equals(serviceFactory) && services.isEmpty()) {
+				problemReporter.reportProblem(annotation, "servicefactory", Messages.AnnotationVisitor_invalidServiceFactory_noServices); //$NON-NLS-1$
+			}
 		}
 
 		Boolean enabled = null;
@@ -540,6 +548,15 @@
 		Boolean immediate = null;
 		if ((value = params.get("immediate")) instanceof Boolean) { //$NON-NLS-1$
 			immediate = (Boolean) value;
+			if (!errorLevel.isIgnore()) {
+				if (factory != null && Boolean.TRUE.equals(immediate)) {
+					problemReporter.reportProblem(annotation, "immediate", Messages.AnnotationVisitor_invalidFactoryComponent_immediate); //$NON-NLS-1$
+				}
+
+				if (services.isEmpty() && Boolean.FALSE.equals(immediate)) {
+					problemReporter.reportProblem(annotation, "immediate", Messages.AnnotationVisitor_invalidDelayedComponent_noServices); //$NON-NLS-1$
+				}
+			}
 		}
 
 		String[] properties;
@@ -607,39 +624,77 @@
 			validateComponentConfigPID(annotation, configPid, -1);
 			requiredVersion = DSAnnotationVersion.V1_2;
 		} else if (specVersion == DSAnnotationVersion.V1_3 && value instanceof Object[]) {
-			// TODO validate empty array and duplicate PIDs!
-			LinkedHashSet<String> configPids = new LinkedHashSet<>(1);
-			int i = 0;
-			for (Object configPidElem : ((Object[]) value)) {
-				String configPidStr = String.valueOf(configPidElem);
-				if ("$".equals(configPidStr)) { //$NON-NLS-1$
-					configPids.add(name);
-				} else {
-					configPids.add(configPidStr);
-					validateComponentConfigPID(annotation, configPidStr, i);
+			Object[] configPidElems = (Object[]) value;
+			if (configPidElems.length > 0) {
+				LinkedHashSet<String> configPids = new LinkedHashSet<>(configPidElems.length);
+				HashMap<String, Integer> pidDuplicates = errorLevel.isIgnore() ? null : new HashMap<>(configPidElems.length);
+				int i = 0;
+				for (Object configPidElem : configPidElems) {
+					String configPidStr = String.valueOf(configPidElem);
+					if ("$".equals(configPidStr)) { //$NON-NLS-1$
+						configPidStr = name;
+					} else {
+						validateComponentConfigPID(annotation, configPidStr, i);
+					}
+
+					if (configPids.add(configPidStr)) {
+						if (pidDuplicates != null) {
+							pidDuplicates.put(configPidStr, i);
+						}
+					} else {
+						if (pidDuplicates != null) {
+							problemReporter.reportProblem(annotation, "configurationPid", i, Messages.AnnotationVisitor_invalidComponentConfigurationPid_duplicate); //$NON-NLS-1$
+							Integer pos = pidDuplicates.put(configPidStr, null);
+							if (pos != null) {
+								problemReporter.reportProblem(annotation, "configurationPid", pos.intValue(), Messages.AnnotationVisitor_invalidComponentConfigurationPid_duplicate); //$NON-NLS-1$
+							}
+						}
+					}
+
+					i++;
 				}
 
-				i++;
-			}
+				requiredVersion = i > 1 ?  DSAnnotationVersion.V1_3 : DSAnnotationVersion.V1_2;
 
-			requiredVersion = i > 1 ?  DSAnnotationVersion.V1_3 : DSAnnotationVersion.V1_2;
+				StringBuilder configPidBuf = new StringBuilder();
+				for (String configPidElem : configPids) {
+					if (configPidBuf.length() > 0) {
+						configPidBuf.append(' ');
+					}
 
-			StringBuilder configPidBuf = new StringBuilder();
-			for (String configPidElem : configPids) {
-				if (configPidBuf.length() > 0) {
-					configPidBuf.append(' ');
+					configPidBuf.append(configPidElem);
 				}
 
-				configPidBuf.append(configPidElem);
+				configPid = configPidBuf.toString();
 			}
-
-			configPid = configPidBuf.toString();
 		}
 
 		String serviceScope = null;
 		if (specVersion == DSAnnotationVersion.V1_3 && (value = params.get("scope")) instanceof IVariableBinding) { //$NON-NLS-1$
 			IVariableBinding serviceScopeBinding = (IVariableBinding) value;
 			serviceScope = DSEnums.getServiceScope(serviceScopeBinding.getName());
+			if (!errorLevel.isIgnore()) {
+				if (services.isEmpty()) {
+					problemReporter.reportProblem(annotation, "scope", Messages.AnnotationVisitor_invalidScope_noServices); //$NON-NLS-1$
+				} else if ((factory != null || Boolean.TRUE.equals(immediate)) && !serviceScope.equals(VALUE_SERVICE_SCOPE_SINGLETON)) {
+					problemReporter.reportProblem(annotation, "scope", Messages.AnnotationVisitor_invalidScope_factoryImmediate); //$NON-NLS-1$
+				}
+			}
+		}
+
+		if (specVersion == DSAnnotationVersion.V1_3 && serviceFactory != null && serviceScope != null && !serviceScope.equals(VALUE_SERVICE_SCOPE_DEFAULT)) {
+			// ignore servicefactory if scope specified and not <<DEFAULT>>
+			if (!errorLevel.isIgnore() && !serviceFactory.equals(VALUE_SERVICE_SCOPE_BUNDLE.equals(serviceScope))) {
+				problemReporter.reportProblem(annotation, "servicefactory", -1, true, errorLevel, Messages.AnnotationVisitor_invalidServiceFactory_ignored); //$NON-NLS-1$
+			}
+
+			serviceFactory = null;
+		}
+
+		if (!errorLevel.isIgnore() && serviceFactory != null && !serviceFactory.equals(Boolean.FALSE) && !services.isEmpty()) {
+			if (factory != null || Boolean.TRUE.equals(immediate)) {
+				problemReporter.reportProblem(annotation, "servicefactory", Messages.AnnotationVisitor_invalidServiceFactory_factoryImmediate); //$NON-NLS-1$
+			}
 		}
 
 		IDSComponent component = model.getDSComponent();
diff --git a/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/Messages.java b/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/Messages.java
index d99955e..9e1e1df 100644
--- a/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/Messages.java
+++ b/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/Messages.java
@@ -126,6 +126,22 @@
 
 	public static String AnnotationProcessor_unknownServiceTypeLabel;
 
+	public static String AnnotationVisitor_invalidComponentConfigurationPid_duplicate;
+
+	public static String AnnotationVisitor_invalidFactoryComponent_immediate;
+
+	public static String AnnotationVisitor_invalidDelayedComponent_noServices;
+
+	public static String AnnotationVisitor_invalidScope_factoryImmediate;
+
+	public static String AnnotationVisitor_invalidScope_noServices;
+
+	public static String AnnotationVisitor_invalidServiceFactory_factoryImmediate;
+
+	public static String AnnotationVisitor_invalidServiceFactory_ignored;
+
+	public static String AnnotationVisitor_invalidServiceFactory_noServices;
+
 	public static String BuildPathMarkerResolutionGenerator_additionalBundleResolution_description;
 
 	public static String BuildPathMarkerResolutionGenerator_additionalBundleResolution_label;
diff --git a/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/messages.properties b/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/messages.properties
index 7f62271..88793bd 100644
--- a/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/messages.properties
+++ b/ds/org.eclipse.pde.ds.annotations/src/org/eclipse/pde/ds/internal/annotations/messages.properties
@@ -63,6 +63,14 @@
 AnnotationProcessor_invalidReference_noImplicitUnbind=No implicit unbind method named ''{0}'' found in implementation class.
 AnnotationProcessor_stringOrEmpty=String (or empty)
 AnnotationProcessor_unknownServiceTypeLabel=<service>
+AnnotationVisitor_invalidComponentConfigurationPid_duplicate=Duplicate configuration PID.
+AnnotationVisitor_invalidFactoryComponent_immediate=Factory component cannot be immediate.
+AnnotationVisitor_invalidDelayedComponent_noServices=Component that does not register services must be immediate.
+AnnotationVisitor_invalidScope_factoryImmediate=Factory or immediate component must have singleton scope.
+AnnotationVisitor_invalidScope_noServices=Scope is not applicable to component with no registered services.
+AnnotationVisitor_invalidServiceFactory_factoryImmediate=Factory or immediate component cannot be a service factory.
+AnnotationVisitor_invalidServiceFactory_ignored=Property 'servicefactory' is ignored when non-default scope is specified.
+AnnotationVisitor_invalidServiceFactory_noServices=Component that does not register services cannot be a service factory.
 BuildPathMarkerResolutionGenerator_additionalBundleResolution_description=Add bundle {0}, which exports DS Annotations, as an Additional Bundle dependency visible at build time but not at runtime.
 BuildPathMarkerResolutionGenerator_additionalBundleResolution_label=Add additional bundle dependency
 BuildPathMarkerResolutionGenerator_extraLibraryResolution_description=Add annotations.jar, located in bundle {0}, as an extra library visible at build time but not at runtime.