Skip to content

Commit

Permalink
Merge pull request #475 from Pieter12345/compile_optimization
Browse files Browse the repository at this point in the history 
BugFix for ambigous command detection.
  • Loading branch information
@LadyCailin
LadyCailin committed Feb 16, 2018
2 parents 81f1858 + 732baef commit 56c5c2b
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 51 deletions.
84 changes: 40 additions & 44 deletions src/main/java/com/laytonsmith/core/Script.java
View file
Expand Up @@ -270,7 +270,6 @@ public Construct eval(ParseTree c, final Environment env) throws CancelCommandEx

StackTraceManager stManager = env.getEnv(GlobalEnv.class).GetStackTraceManager();
boolean addedRootStackElement = false;
boolean caughtException = false;
try {
// If it's an unknown target, this is not user generated code, and we want to skip adding the element here.
if(stManager.isStackEmpty() && !m.getTarget().equals(Target.UNKNOWN)){
Expand Down Expand Up @@ -376,7 +375,6 @@ public Construct eval(ParseTree c, final Environment env) throws CancelCommandEx
} catch(ConfigRuntimeException | ProgramFlowManipulationException e){
if(e instanceof AbstractCREException){
((AbstractCREException)e).freezeStackTraceElements(stManager);
caughtException = true;
}
throw e;
} catch(InvalidEnvironmentException e){
Expand Down Expand Up @@ -486,7 +484,7 @@ public boolean match(String command) {
}
boolean case_sensitive = Prefs.CaseSensitive();
String[] cmds = command.split(" ");
List<String> args = new ArrayList(Arrays.asList(cmds));
List<String> args = new ArrayList<>(Arrays.asList(cmds));
boolean isAMatch = true;
StringBuilder lastVar = new StringBuilder();
int lastJ = 0;
Expand Down Expand Up @@ -559,7 +557,7 @@ public boolean match(String command) {

public List<Variable> getVariables(String command) {
String[] cmds = command.split(" ");
List<String> args = new ArrayList(Arrays.asList(cmds));
List<String> args = new ArrayList<>(Arrays.asList(cmds));

StringBuilder lastVar = new StringBuilder();

Expand Down Expand Up @@ -829,57 +827,55 @@ public void checkAmbiguous(List<Script> scripts) throws ConfigCompileException {
for(Script script : scripts) {
List<Construct> thatCommand = script.cleft;
if(thatCommand == null) {
//it hasn't been compiled yet.
// It hasn't been compiled yet.
return;
}
if(this.cleft == script.cleft) {
//Of course this command is going to match it's own signature
// Of course this command is going to match its own signature.
continue;
}
boolean soFarAMatch = true;
for(int k = 0; k < thisCommand.size(); k++) {
try {

matchScope: {
for(int k = 0; k < thisCommand.size(); k++) {
Construct c1 = thisCommand.get(k);
Construct c2 = thatCommand.get(k);
if(c1.getCType() != c2.getCType() || ((c1 instanceof Variable) && !((Variable) c1).isOptional())) {
soFarAMatch = false;
if(k < thatCommand.size()) {
Construct c2 = thatCommand.get(k);

// Commands are not ambigous if they have unequal commands or strings at
// the same argument position.
if(c1.getCType() == c2.getCType()
&& (c1.getCType() == ConstructType.STRING || c1.getCType() == ConstructType.COMMAND)) {
if(c1.nval() != c2.nval() && (c1.nval() == null || !c1.nval().equals(c2.nval()))) {
break matchScope;
}
}

} else {
//It's a literal, check to see if it's the same literal
if(c1.nval() == null || !c1.val().equals(c2.val())) {
soFarAMatch = false;

// thatCommand is shorter than thisCommand.
// Commands are not ambigous if thisCommand contains a non-variable or a non-optional variable
// after the last Construct in thatCommand.
if(!(c1 instanceof Variable) || (c1 instanceof Variable && !((Variable) c1).isOptional())) {
break matchScope;
} else {
break; // There is no need to loop over later Constructs, the commands are ambigous.
}
}
} catch (IndexOutOfBoundsException e) {
/**
* The two commands:
* /cmd $var1 [$var2]
* /cmd $var1
* would cause this exception to be thrown, but the signatures
* are the same, so the fact that they've matched this far means
* they are ambiguous. However,
* /cmd $var1 $var2
* /cmd $var1
* is not ambiguous
*/
//thatCommand is the short one
if(!(thisCommand.get(k) instanceof Variable)
|| (thisCommand.get(k) instanceof Variable
&& !((Variable) thisCommand.get(k)).isOptional())) {
soFarAMatch = false;

}
}
}
if(thatCommand.size() > thisCommand.size()) {
int k = thisCommand.size();
//thisCommand is the short one
if(!(thatCommand.get(k) instanceof Variable)
|| (thatCommand.get(k) instanceof Variable
&& !((Variable) thatCommand.get(k)).isOptional())) {
soFarAMatch = false;
if(thatCommand.size() > thisCommand.size()) {

// thisCommand is shorter than thatCommand.
// Commands are not ambigous if thatCommand contains a non-variable or a non-optional variable
// after the last Construct in thisCommand.
Construct c2 = thatCommand.get(thisCommand.size());
if(!(c2 instanceof Variable) ||(c2 instanceof Variable && !((Variable) c2).isOptional())) {
break matchScope;
}

}
}

if(soFarAMatch) {

// The signature of thisCommand and thatCommand are ambigous. Throw a compile exception.
String commandThis = "";
for(Construct c : thisCommand) {
commandThis += c.val() + " ";
Expand Down
103 changes: 96 additions & 7 deletions src/test/java/com/laytonsmith/core/MethodScriptCompilerTest.java
View file
Expand Up @@ -29,8 +29,6 @@
import org.junit.Test;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static com.laytonsmith.testing.StaticTest.RunCommand;
import static com.laytonsmith.testing.StaticTest.SRun;
import org.junit.Ignore;
//import org.powermock.api.mockito.PowerMockito;
//import org.powermock.core.classloader.annotations.PowerMockIgnore;
Expand Down Expand Up @@ -82,8 +80,7 @@ public void tearDown() {
@Test
public void testLex() throws Exception {
String config = "/cmd = msg('string')";
List e = null;
e = new ArrayList();
List<Token> e = new ArrayList<>();
//This is the decomposed version of the above config
e.add(new Token(Token.TType.COMMAND, "/cmd", Target.UNKNOWN));
e.add(new Token(Token.TType.WHITESPACE, " ", Target.UNKNOWN));
Expand All @@ -95,12 +92,12 @@ public void testLex() throws Exception {
e.add(new Token(Token.TType.FUNC_END, ")", Target.UNKNOWN));
e.add(new Token(Token.TType.NEWLINE, "\n", Target.UNKNOWN));

List result = MethodScriptCompiler.lex(config, null, false);
List<Token> result = MethodScriptCompiler.lex(config, null, false);
assertEquals(e, result);

String[] badConfigs = {
"'\\q'", //Bad escape sequences
"'\\m'",};
"'\\m'"};
for (String c : badConfigs) {
try {
MethodScriptCompiler.lex(c, null, false);
Expand Down Expand Up @@ -1028,5 +1025,97 @@ public void testLiteralBinary2() throws Exception {
assertEquals("-6", SRun("-0b0110", fakePlayer));
assertEquals("int", SRun("typeof(-0b0110)", fakePlayer));
}


@Test
public void testAmbigousCommandRegistration() {

// Test command name difference.
ambigousCommandRegistrationHelper("/cmd = noop()", "/cmd = noop()", true);
ambigousCommandRegistrationHelper("/cmd = noop()", "/cmd2 = noop()", false);

// Test label difference.
ambigousCommandRegistrationHelper("label1:/cmd = noop()", "label1:/cmd = noop()", true);
ambigousCommandRegistrationHelper("label1:/cmd = noop()", "label2:/cmd = noop()", true);

// Test 1 to 2 optional and non-optional arguments.
ambigousCommandRegistrationHelper("/cmd [$arg] = noop()", "/cmd = noop()", true);
ambigousCommandRegistrationHelper("/cmd $arg = noop()", "/cmd = noop()", false);
ambigousCommandRegistrationHelper("/cmd $arg = noop()", "/cmd [$arg] = noop()", true);
ambigousCommandRegistrationHelper("/cmd $arg = noop()", "/cmd $arg = noop()", true);
ambigousCommandRegistrationHelper("/cmd $arg [$arg2] = noop()", "/cmd $arg = noop()", true);
ambigousCommandRegistrationHelper("/cmd $arg $arg2 = noop()", "/cmd $arg = noop()", false);
ambigousCommandRegistrationHelper("/cmd $arg $arg2 = noop()", "/cmd $arg [$arg2] = noop()", true);
ambigousCommandRegistrationHelper("/cmd $arg $arg2 = noop()", "/cmd $arg $arg2 = noop()", true);

// Test 1 to 2 strings without variables.
ambigousCommandRegistrationHelper("/cmd bla = noop()", "/cmd = noop()", false);
ambigousCommandRegistrationHelper("/cmd bla = noop()", "/cmd bla = noop()", true);
ambigousCommandRegistrationHelper("/cmd bla = noop()", "/cmd bla2 = noop()", false);
ambigousCommandRegistrationHelper("/cmd bla bla2 = noop()", "/cmd bla = noop()", false);
ambigousCommandRegistrationHelper("/cmd bla bla2 = noop()", "/cmd bla bla2 = noop()", true);
ambigousCommandRegistrationHelper("/cmd bla bla2 = noop()", "/cmd bla3 bla2 = noop()", false);

// Test collision between strings and variables.
ambigousCommandRegistrationHelper("/cmd bla = noop()", "/cmd $arg = noop()", true);
ambigousCommandRegistrationHelper("/cmd bla = noop()", "/cmd [$arg] = noop()", true);
ambigousCommandRegistrationHelper("/cmd bla bla2 = noop()", "/cmd bla [$arg] = noop()", true);
ambigousCommandRegistrationHelper("/cmd bla bla2 = noop()", "/cmd bla $arg = noop()", true);

}

/**
* Checks if cmd1 and cmd2 would cause a ConfigCompileException due to being ambigous.
* Calls "fail(message)" when compile success != expectFail.
* The order of cmd1 and cmd2 does not matter, this method checks both orders.
* @param cmd1 - The first command.
* @param cmd2 - The second command.
* @param expectFail - True if cmd1 and cmd2 are expected to be ambigous, false otherwise.
*/
private void ambigousCommandRegistrationHelper(String cmd1, String cmd2, boolean expectFail) {
Script s1, s2;
try {
List<Script> scripts = MethodScriptCompiler.preprocess(
MethodScriptCompiler.lex(cmd1 + "\n" + cmd2, null, false));
s1 = scripts.get(0);
s2 = scripts.get(1);
s1.compile();
s2.compile();
} catch (ConfigCompileGroupException | ConfigCompileException | IndexOutOfBoundsException e) {
fail("Encountered unexpected " + e.getClass().getSimpleName() + " with message: " + e.getMessage());
return;
}

// Check scripts 1 and 2 against eachother.
ConfigCompileException e1 = null, e2 = null;
try {
s2.checkAmbiguous(Arrays.asList(new Script[] {s1}));
} catch (ConfigCompileException e) {
e1 = e;
}
try {
s1.checkAmbiguous(Arrays.asList(new Script[] {s2}));
} catch (ConfigCompileException e) {
e2 = e;
}

// Fail if the behaviour of script 1 VS 2 did not match script 2 VS 1.
if((e1 == null) != (e2 == null)) {
fail("Commands '" + cmd1 + "' and '" + cmd2 + "' are both ambigous and non-ambigous"
+ " depending on the command order. Since command order does not matter, this is a bug.");
}

// Fail if the result differs from the expected result.
if(expectFail) {
if(e1 == null) {
fail("Commands '" + cmd1 + "' and '" + cmd2 + "' are expected to be ambigous,"
+ " but they did not cause the expected ConfigCompileException.");
}
} else {
if(e1 != null) {
fail("Commands '" + cmd1 + "' and '" + cmd2 + "' are expected to be non-ambigous,"
+ " but they did cause a ConfigCompileException with message: " + e1.getMessage());
}
}
}

}

0 comments on commit 56c5c2b

Please sign in to comment.