Skip to content

Commit

Permalink
GH-2259: Package.json validations showing bogus errors in incremental…
Browse files Browse the repository at this point in the history
… headless build (#2260)

* avoid "load from source" in a package.json validation

* clean up

* improve a comment

* add a test
  • Loading branch information
mor-n4 authored Nov 10, 2021
1 parent c04ac36 commit ff56300
Show file tree
Hide file tree
Showing 3 changed files with 291 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import org.apache.log4j.Logger
import org.eclipse.emf.common.util.URI
import org.eclipse.emf.ecore.EObject
import org.eclipse.emf.ecore.resource.Resource
import org.eclipse.emf.ecore.util.EcoreUtil
import org.eclipse.n4js.N4JSGlobals
import org.eclipse.n4js.json.JSON.JSONArray
import org.eclipse.n4js.json.JSON.JSONDocument
Expand Down Expand Up @@ -71,6 +70,7 @@ import org.eclipse.n4js.validation.helper.SourceContainerAwareDependencyProvider
import org.eclipse.n4js.workspace.N4JSProjectConfigSnapshot
import org.eclipse.n4js.workspace.N4JSWorkspaceConfigSnapshot
import org.eclipse.n4js.workspace.WorkspaceAccess
import org.eclipse.n4js.workspace.utils.N4JSPackageName
import org.eclipse.xtend.lib.annotations.Data
import org.eclipse.xtext.nodemodel.util.NodeModelUtils
import org.eclipse.xtext.resource.IContainer
Expand All @@ -87,7 +87,6 @@ import static org.eclipse.n4js.validation.IssueCodes.*
import static org.eclipse.n4js.validation.validators.packagejson.ProjectTypePredicate.*

import static extension com.google.common.base.Strings.nullToEmpty
import org.eclipse.n4js.workspace.utils.N4JSPackageName

/**
* A JSON validator extension that validates {@code package.json} resources in the context
Expand Down Expand Up @@ -225,6 +224,12 @@ public class N4JSProjectSetupJsonValidatorExtension extends AbstractPackageJSONV
}
}

val xtextIndex = workspaceAccess.getXtextIndex(document).orNull;
val contextResourceSet = document.eResource?.resourceSet;
if (xtextIndex === null || contextResourceSet === null) {
return;
}

// Search for clashes in Polyfill:
// markermap: {lib1,lib2,...}->"filledname"
val Multimap<Set<JSONStringLiteral>, String> markerMapLibs2FilledName = LinkedListMultimap.create // Value is QualifiedName of polyfill_Element
Expand All @@ -240,8 +245,19 @@ public class N4JSProjectSetupJsonValidatorExtension extends AbstractPackageJSONV
var eoPolyFiller = prov.ieoDescrOfPolyfill.EObjectOrProxy

if (eoPolyFiller instanceof TClassifier) {
val resolvedEoPolyFiller = EcoreUtil.resolve(eoPolyFiller, document.eResource) as TClassifier
if (!resolvedEoPolyFiller.isPolyfill) {
// WARNING: simply doing
// val resolvedEoPolyFiller = EcoreUtil.resolve(eoPolyFiller, document.eResource);
// here would result in the resource of eoPolyFiller being loaded from source, because
// we are in the context of a package.json file (not an N4JSResource, etc.) and therefore
// do not get automatic "load from index" behavior!
var resolvedEoPolyFiller = eoPolyFiller;
if (resolvedEoPolyFiller.eIsProxy) {
val targetObjectURI = prov.ieoDescrOfPolyfill.EObjectURI;
resolvedEoPolyFiller = workspaceAccess.loadEObjectFromIndex(xtextIndex, contextResourceSet, targetObjectURI, false) as TClassifier;
}
if (resolvedEoPolyFiller === null || resolvedEoPolyFiller.eIsProxy) {
// unable to resolve -> ignore
} else if (!resolvedEoPolyFiller.isPolyfill) {
throw new IllegalStateException(
"Expected a polyfill, but wasn't: " + resolvedEoPolyFiller.name)
} else { // yes, it's an polyfiller.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@
import org.eclipse.emf.common.notify.Notifier;
import org.eclipse.emf.common.util.URI;
import org.eclipse.emf.ecore.EObject;
import org.eclipse.emf.ecore.InternalEObject;
import org.eclipse.emf.ecore.resource.Resource;
import org.eclipse.emf.ecore.resource.ResourceSet;
import org.eclipse.n4js.n4JS.Script;
import org.eclipse.n4js.resource.N4JSResource;
import org.eclipse.n4js.scoping.utils.UserDataAwareScope;
import org.eclipse.n4js.ts.types.TModule;
import org.eclipse.n4js.utils.ResourceType;
import org.eclipse.n4js.utils.emf.ProxyResolvingEObjectImpl;
import org.eclipse.n4js.workspace.utils.N4JSPackageName;
import org.eclipse.n4js.xtext.workspace.ProjectSet;
import org.eclipse.n4js.xtext.workspace.WorkspaceConfigAdapter;
Expand Down Expand Up @@ -183,6 +187,58 @@ public Optional<IResourceDescriptions> getXtextIndex(Notifier context) {
return Optional.fromNullable(result);
}

/**
* Convenience method. When looking up several objects, prefer method
* {@link #loadEObjectFromIndex(IResourceDescriptions, ResourceSet, URI, boolean)} (for performance reasons).
*/
public EObject loadEObjectFromIndex(ResourceSet resourceSet, URI eObjectURI, boolean allowFullLoad) {
Optional<IResourceDescriptions> index = getXtextIndex(resourceSet);
if (!index.isPresent()) {
return null;
}
return loadEObjectFromIndex(index.get(), resourceSet, eObjectURI, allowFullLoad);
}

/**
* Loads a TModule element from the user data of the Xtext index. Returns <code>null</code> in case of error. If the
* given <code>eObjectURI</code> does not point to a TModule element, this method will avoid a "load from source"
* and instead return <code>null</code>.
* <p>
* Normally the "load from index" behavior should happen automatically (via {@link UserDataAwareScope} and/or
* {@link ProxyResolvingEObjectImpl} with {@link N4JSResource#doResolveProxy(InternalEObject, EObject)}), so this
* method should only be used in rare special cases if there is a good reason for the automatic "load from index"
* behavior being unavailable.
*
* @param index
* the Xtext index to use.
* @param resourceSet
* the resource set to used as context and as container for the newly created resource containing the
* target object. This resource set may be changed!
* @param eObjectURI
* the URI of the target object to load.
* @param allowFullLoad
* see
* @return the object loaded or <code>null</code>.
*/
public EObject loadEObjectFromIndex(IResourceDescriptions index, ResourceSet resourceSet, URI eObjectURI,
boolean allowFullLoad) {
URI targetResourceURI = eObjectURI.trimFragment();
ResourceType targetResourceType = ResourceType.getResourceType(targetResourceURI);
String targetFragment = eObjectURI.fragment();
if (!targetResourceType.isN4JS()
|| targetFragment == null
|| !(targetFragment.equals("/1") || targetFragment.startsWith("/1/"))) {
// eObjectURI does not point to an EObject in the TModule of an N4JS resource
return null;
}
IResourceDescription targetResourceDesc = index.getResourceDescription(targetResourceURI);
TModule targetModule = targetResourceDesc != null
? loadModuleFromIndex(resourceSet, targetResourceDesc, allowFullLoad)
: null;
Resource targetResource = targetModule != null ? targetModule.eResource() : null;
return targetResource != null ? targetResource.getEObject(targetFragment) : null;
}

/**
* Deserialize the TModule stored in the user data of the Xtext index.
* <p>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
/**
* Copyright (c) 2021 NumberFour AG.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* NumberFour AG - Initial API and implementation
*/
package org.eclipse.n4js.ide.tests.validation

import com.google.common.base.Optional
import java.io.IOException
import java.util.Collections
import java.util.List
import org.eclipse.emf.ecore.resource.ResourceSet
import org.eclipse.n4js.ide.server.build.N4JSProjectBuilder
import org.eclipse.n4js.ide.tests.helper.server.AbstractIdeTest
import org.eclipse.n4js.resource.N4JSResource
import org.eclipse.n4js.xtext.ide.server.build.IBuildRequestFactory
import org.eclipse.n4js.xtext.ide.server.build.ProjectBuilder
import org.eclipse.n4js.xtext.ide.server.build.XBuildResult
import org.eclipse.xtext.resource.IResourceDescription.Delta
import org.eclipse.xtext.service.AbstractGenericModule
import org.junit.Test

import static org.junit.Assert.*

/**
* Test for bug GH-2259.
* <p>
* The problem was that a package.json validation caused resources in other projects to be loaded from source
* (in method {@code N4JSProjectSetupJsonValidatorExtension#checkConsistentPolyfills(JSONDocument)}).
*/
class GH_2259_PackageJsonValidationMustNotLoadFromSourceTest extends AbstractIdeTest {

// The custom implementation of N4JSProjectBuilder (see below) will add messages to
// this list while the initial build is running:
private static final List<String> buildProgressMessages = Collections.synchronizedList(newArrayList);

@Test
def void test() throws IOException {
testWorkspaceManager.createTestOnDisk(
"Project1" -> #[
"SomeModule1.n4jsd" -> '''
@@Global @@ProvidedByRuntime
@Polyfill
export external public class Array<T> extends Array<T> {
public addedMethod();
}
''',
PACKAGE_JSON -> '''
{
"name": "Project1",
"version": "0.0.1",
"n4js": {
"projectType": "runtimeLibrary",
"vendorId": "org.eclipse.n4js",
"sources": {
"source": [
"src"
]
},
"output": "src-gen"
}
}
'''
],
"Project2" -> #[
"SomeModule2.n4jsd" -> '''
@@Global @@ProvidedByRuntime
@Polyfill
export external public class Array<T> extends Array<T> {
public addedMethod();
}
''',
PACKAGE_JSON -> '''
{
"name": "Project2",
"version": "0.0.1",
"n4js": {
"projectType": "runtimeLibrary",
"vendorId": "org.eclipse.n4js",
"sources": {
"source": [
"src"
]
},
"output": "src-gen"
}
}
'''
],
"MainProject" -> #[
"Main.n4jsd" -> '''
export external public class MainClass {}
''',
CFG_DEPENDENCIES -> '''
Project1
Project2
''',
PACKAGE_JSON -> '''
{
"name": "MainProject",
"version": "0.0.1",
"n4js": {
"projectType": "runtimeLibrary",
"vendorId": "org.eclipse.n4js",
"sources": {
"source": [
"src"
]
},
"output": "src-gen",
"requiredRuntimeLibraries": [
"Project1",
"Project2"
]
},
"dependencies": {
"Project1": "*",
"Project2": "*"
}
}
'''
]
);

buildProgressMessages.clear();

startAndWaitForLspServer();
assertIssues(
"MainProject/package.json" -> #[
'(Error, [13:3 - 13:13], The libraries Project1, Project2 provide polyfills for the same element "#.Array#addedMethod" and cannot be used together.)',
'(Error, [14:3 - 14:13], The libraries Project1, Project2 provide polyfills for the same element "#.Array#addedMethod" and cannot be used together.)'
]
);

assertEquals(8, buildProgressMessages.size);

val messagesOfBuildingMainProject = buildProgressMessages.drop(6).map[trim].join("\n-----\n");

assertEquals('''
BEFORE building project "yarn-test-project/packages/MainProject"
Resource set of Project1 contains no resources
Resource set of Project2 contains no resources
-----
AFTER building project "yarn-test-project/packages/MainProject"
Resource set of Project1 contains a single resource loaded from description
Resource set of Project2 contains a single resource loaded from description
'''.toString.trim, messagesOfBuildingMainProject);
// NOTE: before the bug fix, resources in Project1 and Project2 were loaded from source while
// building project "MainProject", so we would see the string segment "loaded from source" instead
// of "loaded from description".
}

override protected getOverridingModule() {
return Optional.of(GH_2259_TestModule);
}

public static final class GH_2259_TestModule extends AbstractGenericModule {

def Class<? extends ProjectBuilder> bindProjectBuilder() {
return GH_2259_TestProjectBuilder;
}
}

public static final class GH_2259_TestProjectBuilder extends N4JSProjectBuilder {

override XBuildResult doInitialBuild(IBuildRequestFactory buildRequestFactory, List<Delta> externalDeltas) {
val id = getProjectConfig().getName();
val pb1 = this.workspaceManager.getProjectBuilder("yarn-test-project/packages/Project1");
val pb2 = this.workspaceManager.getProjectBuilder("yarn-test-project/packages/Project2");

buildProgressMessages += '''
BEFORE building project "«id»"
Resource set of Project1 «getResourceSetStatus(pb1.resourceSet)»
Resource set of Project2 «getResourceSetStatus(pb2.resourceSet)»
''';

val result = super.doInitialBuild(buildRequestFactory, externalDeltas);

buildProgressMessages += '''
AFTER building project "«id»"
Resource set of Project1 «getResourceSetStatus(pb1.resourceSet)»
Resource set of Project2 «getResourceSetStatus(pb2.resourceSet)»
''';

return result;
}

def private String getResourceSetStatus(ResourceSet resSet) {
val resources = resSet.resources;
return if (resources.empty) {
"contains no resources"
} else if(resources.size === 1) {
val res = resources.head as N4JSResource;
"contains a single resource" + (
if (res.getScript() !== null && !res.getScript().eIsProxy()) {
" loaded from source"
} else if (res.isLoadedFromDescription()) {
" loaded from description"
} else {
""
}
)
} else {
"contains " + resources.size + " resources"
}
}
}
}

0 comments on commit ff56300

Please sign in to comment.