Skip to content

Commit

Permalink
Make System.getProperty fail at compile time for undefined properties.
Browse files Browse the repository at this point in the history 
System.getProperty is designed to enable dead code stripping, so give an
early error if a property is used but not declared.

This patch also refactors the test infrastructure a bit to allow some
reused of CheckerTests by UnifyAstTest.

Change-Id: I7b24575f14e76ed74c32872bd9699155fa8c9cdd
  • Loading branch information
@rluble
rluble committed Jun 4, 2015
1 parent e6c2f2c commit 3ba256a
Show file tree
Hide file tree
Showing 15 changed files with 319 additions and 123 deletions.
12 changes: 11 additions & 1 deletion dev/core/src/com/google/gwt/dev/javac/testing/impl/JavaResourceBase.java
View file
Expand Up @@ -345,6 +345,16 @@ public class JavaResourceBase {
" String[] value();", " String[] value();",
"}"); "}");


public static final MockJavaResource SYSTEM =
createMockJavaResource("java.lang.System",
"package java.lang;",
"public class System {",
" public static String getProperty(String propertyName) { return null; }",
" public static String getProperty(String propertyName, String defaultValue) {",
" return defaultValue;",
" }",
"}");

public static final MockJavaResource THROWABLE = public static final MockJavaResource THROWABLE =
createMockJavaResource("java.lang.Throwable", createMockJavaResource("java.lang.Throwable",
"package java.lang;", "package java.lang;",
Expand Down Expand Up @@ -400,7 +410,7 @@ public static MockJavaResource[] getStandardResources() {
CLASS_NOT_FOUND_EXCEPTION, CLONEABLE, COLLECTION, COMPARABLE, DOUBLE, ENUM, EXCEPTION, CLASS_NOT_FOUND_EXCEPTION, CLONEABLE, COLLECTION, COMPARABLE, DOUBLE, ENUM, EXCEPTION,
ERROR, FUNCTIONALINTERFACE, FLOAT, INTEGER, IS_SERIALIZABLE, JAVASCRIPTEXCEPTION, ERROR, FUNCTIONALINTERFACE, FLOAT, INTEGER, IS_SERIALIZABLE, JAVASCRIPTEXCEPTION,
JAVASCRIPTOBJECT, LIST, LONG, MAP, NO_CLASS_DEF_FOUND_ERROR, NUMBER, OBJECT, JAVASCRIPTOBJECT, LIST, LONG, MAP, NO_CLASS_DEF_FOUND_ERROR, NUMBER, OBJECT,
RUNTIME_EXCEPTION, SERIALIZABLE, SHORT, STRING, STRING_BUILDER, SUPPRESS_WARNINGS, RUNTIME_EXCEPTION, SERIALIZABLE, SHORT, STRING, STRING_BUILDER, SUPPRESS_WARNINGS, SYSTEM,
THROWABLE, SPECIALIZE_METHOD, JSTYPE, JSTYPEPROTOTYPE, JSEXPORT, JSPROPERTY, JSFUNCTION}; THROWABLE, SPECIALIZE_METHOD, JSTYPE, JSTYPEPROTOTYPE, JSEXPORT, JSPROPERTY, JSFUNCTION};
} }


Expand Down
18 changes: 12 additions & 6 deletions dev/core/src/com/google/gwt/dev/jjs/AstConstructor.java
View file
Expand Up @@ -40,19 +40,25 @@
*/ */
public class AstConstructor { public class AstConstructor {


public static JProgram construct(TreeLogger logger, final CompilationState state,
PrecompileTaskOptions options, ConfigurationProperties config)
throws UnableToCompleteException {
CompilerContext compilerContext = new CompilerContext.Builder().options(options)
.minimalRebuildCache(new NullRebuildCache()).build();
return construct(logger, state, compilerContext, config);
}

/** /**
* Construct an simple AST representing an entire {@link CompilationState}. * Construct an simple AST representing an entire {@link CompilationState}.
* Does not support deferred binding. Implementation mostly copied from * Does not support deferred binding. Implementation mostly copied from
* {@link JavaToJavaScriptCompiler}. * {@link JavaToJavaScriptCompiler}.
*/ */
public static JProgram construct(TreeLogger logger, final CompilationState state, public static JProgram construct(TreeLogger logger, final CompilationState state,
PrecompileTaskOptions options, ConfigurationProperties config) throws UnableToCompleteException { CompilerContext compilerContext, ConfigurationProperties config)
throws UnableToCompleteException {


InternalCompilerException.preload(); InternalCompilerException.preload();


CompilerContext compilerContext = new CompilerContext.Builder().options(options)
.minimalRebuildCache(new NullRebuildCache()).build();

PrecompilationContext precompilationContext = new PrecompilationContext( PrecompilationContext precompilationContext = new PrecompilationContext(
new RebindPermutationOracle() { new RebindPermutationOracle() {
@Override @Override
Expand Down Expand Up @@ -93,15 +99,15 @@ public StandardGeneratorContext getGeneratorContext() {
* TODO: If we defer this until later, we could maybe use the results of the * TODO: If we defer this until later, we could maybe use the results of the
* assertions to enable more optimizations. * assertions to enable more optimizations.
*/ */
if (options.isEnableAssertions()) { if (compilerContext.getOptions().isEnableAssertions()) {
// Turn into assertion checking calls. // Turn into assertion checking calls.
AssertionNormalizer.exec(jprogram); AssertionNormalizer.exec(jprogram);
} else { } else {
// Remove all assert statements. // Remove all assert statements.
AssertionRemover.exec(jprogram); AssertionRemover.exec(jprogram);
} }


if (options.isRunAsyncEnabled()) { if (compilerContext.getOptions().isRunAsyncEnabled()) {
ReplaceRunAsyncs.exec(logger, jprogram); ReplaceRunAsyncs.exec(logger, jprogram);
if (config != null) { if (config != null) {
CodeSplitters.pickInitialLoadSequence(logger, jprogram, config); CodeSplitters.pickInitialLoadSequence(logger, jprogram, config);
Expand Down
29 changes: 23 additions & 6 deletions dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java
View file
Expand Up @@ -21,6 +21,8 @@
import com.google.gwt.dev.CompilerContext; import com.google.gwt.dev.CompilerContext;
import com.google.gwt.dev.MinimalRebuildCache; import com.google.gwt.dev.MinimalRebuildCache;
import com.google.gwt.dev.Permutation; import com.google.gwt.dev.Permutation;
import com.google.gwt.dev.cfg.ConfigurationProperty;
import com.google.gwt.dev.cfg.Property;
import com.google.gwt.dev.javac.CompilationProblemReporter; import com.google.gwt.dev.javac.CompilationProblemReporter;
import com.google.gwt.dev.javac.CompilationState; import com.google.gwt.dev.javac.CompilationState;
import com.google.gwt.dev.javac.CompilationUnit; import com.google.gwt.dev.javac.CompilationUnit;
Expand Down Expand Up @@ -413,22 +415,30 @@ public boolean visit(JMethodCall x, Context ctx) {
private JExpression handleSystemGetProperty(JMethodCall gwtGetPropertyCall) { private JExpression handleSystemGetProperty(JMethodCall gwtGetPropertyCall) {
assert (gwtGetPropertyCall.getArgs().size() == 1 || gwtGetPropertyCall.getArgs().size() == 2); assert (gwtGetPropertyCall.getArgs().size() == 1 || gwtGetPropertyCall.getArgs().size() == 2);
JExpression propertyNameExpression = gwtGetPropertyCall.getArgs().get(0); JExpression propertyNameExpression = gwtGetPropertyCall.getArgs().get(0);
JExpression defaultValueExpression = gwtGetPropertyCall.getArgs().size() == 2 ? boolean defaultVersionCalled = gwtGetPropertyCall.getArgs().size() == 2;
JExpression defaultValueExpression = defaultVersionCalled ?
gwtGetPropertyCall.getArgs().get(1) : null; gwtGetPropertyCall.getArgs().get(1) : null;


if (!(propertyNameExpression instanceof JStringLiteral) || if (!(propertyNameExpression instanceof JStringLiteral) ||
(defaultValueExpression != null && !(defaultValueExpression instanceof JStringLiteral))) { (defaultVersionCalled && !(defaultValueExpression instanceof JStringLiteral))) {
error(gwtGetPropertyCall, error(gwtGetPropertyCall,
"Only string constants may be used as arguments to System.getProperty()"); "Only string constants may be used as arguments to System.getProperty()");
return null; return null;
} }
String propertyName = ((JStringLiteral) propertyNameExpression).getValue(); String propertyName = ((JStringLiteral) propertyNameExpression).getValue();


if (!defaultVersionCalled && !isPropertyDefined(propertyName)) {
error(gwtGetPropertyCall, "Property '" + propertyName + "' is not defined.");
return null;
}

if (isMultivaluedProperty(propertyName)) { if (isMultivaluedProperty(propertyName)) {
error(gwtGetPropertyCall, error(gwtGetPropertyCall,
"Multivalued properties are not supported by System.getProperty()"); "Property '" + propertyName + "' is multivalued. " +
"Multivalued properties are not supported by System.getProperty().");
return null; return null;
} }

String defaultValue = defaultValueExpression == null ? null : String defaultValue = defaultValueExpression == null ? null :
((JStringLiteral) defaultValueExpression).getValue(); ((JStringLiteral) defaultValueExpression).getValue();
return JPermutationDependentValue return JPermutationDependentValue
Expand Down Expand Up @@ -591,9 +601,16 @@ private JExpression handleMagicMethodCall(JMethodCall x, String targetSignature)
} }


private boolean isMultivaluedProperty(String propertyName) { private boolean isMultivaluedProperty(String propertyName) {
// Multivalued properties can only be Configuration properties, and those do not change between Property property = compilerContext.getModule().getProperties().find(propertyName);
// permutations. if (!(property instanceof ConfigurationProperty)) {
return permutations[0].getProperties().getConfigurationProperties().isMultiValued(propertyName); return false;
}

return ((ConfigurationProperty) property).allowsMultipleValues();
}

private boolean isPropertyDefined(String propertyName) {
return compilerContext.getModule().getProperties().find(propertyName) != null;
} }


private static final String CLASS_DESIRED_ASSERTION_STATUS = private static final String CLASS_DESIRED_ASSERTION_STATUS =
Expand Down
7 changes: 7 additions & 0 deletions dev/core/test/com/google/gwt/dev/cfg/MockModuleDef.java
View file
Expand Up @@ -56,9 +56,16 @@ public boolean wasRerooted() {
} }
}; };


private Properties properties = new Properties();

public MockModuleDef() { public MockModuleDef() {
super("mock"); super("mock");
normalize(TreeLogger.NULL); normalize(TreeLogger.NULL);
lazyPublicOracle = new MockResourceOracle(publicResource); lazyPublicOracle = new MockResourceOracle(publicResource);
} }

@Override
public Properties getProperties() {
return properties;
}
} }
98 changes: 62 additions & 36 deletions dev/core/test/com/google/gwt/dev/javac/CheckerTestCase.java
View file
Expand Up @@ -23,17 +23,29 @@
import com.google.gwt.dev.resource.Resource; import com.google.gwt.dev.resource.Resource;
import com.google.gwt.dev.util.UnitTestTreeLogger; import com.google.gwt.dev.util.UnitTestTreeLogger;
import com.google.gwt.thirdparty.guava.common.base.Joiner; import com.google.gwt.thirdparty.guava.common.base.Joiner;
import com.google.gwt.thirdparty.guava.common.collect.Sets;


import junit.framework.TestCase; import junit.framework.TestCase;


import java.util.HashSet;
import java.util.Set; import java.util.Set;


/** /**
* A base type for testing the code checkers. * A base type for testing the code checkers.
*/ */
public abstract class CheckerTestCase extends TestCase { public abstract class CheckerTestCase extends TestCase {


/**
* Pass represents passes to be run by the checker test cases.
*/
public interface Pass {
/**
* Run the pass. Returns true if pass completes successfully.
*/
boolean run(TreeLogger logger, MockJavaResource buggyResource, MockJavaResource extraResource);

boolean classAvailable(String className);
String getTopErrorMessage(Type logLevel, MockJavaResource resource);
}
/** /**
* A warning or error message. * A warning or error message.
*/ */
Expand All @@ -49,6 +61,41 @@ private Message(int line, Type logLevel, String message) {
} }
} }


/**
* Provide a pass to be checked by default runs only passes required to build TypeOracle.
*/
protected Pass providePass() {
return new Pass() {
private TypeOracle oracle = null;
@Override
public boolean run(TreeLogger logger, MockJavaResource buggyResource,
MockJavaResource extraResource) {
Set<Resource> resources = Sets.newHashSet();
addLongCheckingCups(resources);
Set<GeneratedUnit> generatedUnits =
CompilationStateTestBase.getGeneratedUnits(buggyResource);
if (extraResource != null) {
generatedUnits.addAll(CompilationStateTestBase.getGeneratedUnits(extraResource));
}
CompilationState state = TypeOracleTestingUtils.buildCompilationStateWith(logger,
resources, generatedUnits);
oracle = state.getTypeOracle();
return !state.hasErrors();
}

@Override
public boolean classAvailable(String className) {
return oracle.findType(className) != null;
}

@Override
public String getTopErrorMessage(Type logLevel, MockJavaResource resource) {
return (logLevel == Type.WARN ? "Warnings" : "Errors") +
" in '" + resource.getLocation() + "'";
}
};
}

protected Message warning(int line, String message) { protected Message warning(int line, String message) {
return new Message(line, Type.WARN, message); return new Message(line, Type.WARN, message);
} }
Expand All @@ -61,40 +108,39 @@ protected void shouldGenerate(MockJavaResource buggyCode, MockJavaResource extra
Type logLevel, Message... messages) { Type logLevel, Message... messages) {
UnitTestTreeLogger.Builder b = new UnitTestTreeLogger.Builder(); UnitTestTreeLogger.Builder b = new UnitTestTreeLogger.Builder();
b.setLowestLogLevel(logLevel); b.setLowestLogLevel(logLevel);
if (messages.length != 0) { Pass pass = providePass();
b.expect(logLevel, (logLevel == Type.WARN ? "Warnings" : "Errors") + String topLevelMessage = pass.getTopErrorMessage(logLevel, buggyCode);
" in '" + buggyCode.getLocation() + "'", null); if (messages.length != 0 && topLevelMessage != null) {
b.expect(logLevel, topLevelMessage, null);
} }
for (Message message : messages) { for (Message message : messages) {
final String fullMessage = "Line " + message.line + ": " + message.message; final String fullMessage = "Line " + message.line + ": " + message.message;
b.expect(message.logLevel, fullMessage, null); b.expect(message.logLevel, fullMessage, null);
} }
UnitTestTreeLogger logger = b.createLogger(); UnitTestTreeLogger logger = b.createLogger();
TypeOracle oracle = buildOracle(logger, buggyCode, extraCode);
boolean result = pass.run(logger, buggyCode, extraCode);


logger.assertCorrectLogEntries(); logger.assertCorrectLogEntries();
String className = buggyCode.getTypeName(); String className = buggyCode.getTypeName();
if (messages.length != 0 && logLevel == TreeLogger.ERROR) { if (messages.length != 0 && logLevel == TreeLogger.ERROR) {
assertNull("Buggy compilation unit not removed from type oracle", assertFalse("Compilation unit " + className + " not removed" +
oracle.findType(className)); " but should have been removed.",
pass.classAvailable(className));
} else { } else {
assertNotNull("Buggy compilation unit removed with only a warning", assertTrue("Compilation unit " + className + " was removed but shouldnt have.",
oracle.findType(className)); pass.classAvailable(className));
} }

boolean expectingErrors = messages.length != 0 && logLevel == Type.ERROR;
assertEquals(!expectingErrors, result);
} }


protected void shouldGenerateError(MockJavaResource buggyCode, MockJavaResource extraCode, protected void shouldGenerateError(MockJavaResource buggyCode, MockJavaResource extraCode,
int line, String message) { int line, String message) {
shouldGenerate(buggyCode, extraCode, TreeLogger.ERROR, error(line, message)); shouldGenerate(buggyCode, extraCode, TreeLogger.ERROR, error(line, message));
} }


protected void shouldGenerateError(CharSequence buggyCode, CharSequence extraCode, int line,
String message) {
StaticJavaResource codeResource = new StaticJavaResource("Buggy", buggyCode);
StaticJavaResource extraResource = new StaticJavaResource("Extra", extraCode);
shouldGenerate(codeResource, extraResource, TreeLogger.ERROR, error(line, message));
}

protected void shouldGenerateError(MockJavaResource buggyCode, int line, String message) { protected void shouldGenerateError(MockJavaResource buggyCode, int line, String message) {
shouldGenerateError(buggyCode, null, line, message); shouldGenerateError(buggyCode, null, line, message);
} }
Expand All @@ -107,12 +153,6 @@ protected void shouldGenerateNoError(MockJavaResource code, MockJavaResource ext
shouldGenerate(code, extraCode, TreeLogger.ERROR); shouldGenerate(code, extraCode, TreeLogger.ERROR);
} }


protected void shouldGenerateNoError(CharSequence code, CharSequence extraCode) {
StaticJavaResource codeResource = new StaticJavaResource("Buggy", code);
StaticJavaResource extraResource = new StaticJavaResource("Extra", extraCode);
shouldGenerate(codeResource, extraResource, TreeLogger.ERROR);
}

protected void shouldGenerateNoWarning(MockJavaResource code) { protected void shouldGenerateNoWarning(MockJavaResource code) {
shouldGenerateNoWarning(code, null); shouldGenerateNoWarning(code, null);
} }
Expand All @@ -139,7 +179,6 @@ protected void shouldGenerateWarnings(MockJavaResource buggyCode, MockJavaResour
Message... messages) { Message... messages) {
shouldGenerate(buggyCode, extraCode, TreeLogger.WARN, messages); shouldGenerate(buggyCode, extraCode, TreeLogger.WARN, messages);
} }
private String buggyPackage = "";


private void addLongCheckingCups(Set<Resource> resources) { private void addLongCheckingCups(Set<Resource> resources) {
String code = Joiner.on('\n').join( String code = Joiner.on('\n').join(
Expand All @@ -149,17 +188,4 @@ private void addLongCheckingCups(Set<Resource> resources) {
resources.add(new StaticJavaResource( resources.add(new StaticJavaResource(
"com.google.gwt.core.client.UnsafeNativeLong", code.toString())); "com.google.gwt.core.client.UnsafeNativeLong", code.toString()));
} }

private TypeOracle buildOracle(UnitTestTreeLogger logger, MockJavaResource buggyResource,
MockJavaResource extraResource) {
Set<Resource> resources = new HashSet<Resource>();
addLongCheckingCups(resources);
Set<GeneratedUnit> generatedUnits = CompilationStateTestBase.getGeneratedUnits(buggyResource);
if (extraResource != null) {
generatedUnits.addAll(CompilationStateTestBase.getGeneratedUnits(extraResource));
}
return TypeOracleTestingUtils.buildStandardTypeOracleWith(logger,
resources, generatedUnits);
}

} }

0 comments on commit 3ba256a

Please sign in to comment.