-
Notifications
You must be signed in to change notification settings - Fork 829
TINKERPOP-3040 Fixed limitation in multi-line parsing. #3162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,11 +23,10 @@ import org.apache.groovy.groovysh.Command | |
import org.apache.groovy.groovysh.Groovysh | ||
import org.apache.groovy.groovysh.ParseCode | ||
import org.apache.groovy.groovysh.Parser | ||
import org.apache.groovy.groovysh.Parsing | ||
import org.apache.groovy.groovysh.util.CommandArgumentParser | ||
import org.apache.groovy.groovysh.util.ScriptVariableAnalyzer | ||
import org.apache.tinkerpop.gremlin.console.commands.GremlinSetCommand | ||
import org.codehaus.groovy.control.CompilerConfiguration | ||
import org.codehaus.groovy.control.MultipleCompilationErrorsException | ||
import org.codehaus.groovy.control.customizers.ASTTransformationCustomizer | ||
import org.codehaus.groovy.tools.shell.IO | ||
|
||
|
@@ -42,6 +41,7 @@ class GremlinGroovysh extends Groovysh { | |
private final static CompilerConfiguration compilerConfig = new CompilerConfiguration(CompilerConfiguration.DEFAULT) {{ | ||
addCompilationCustomizers(new ASTTransformationCustomizer(ThreadInterrupt.class)) | ||
}} | ||
private Parsing remoteParser = new LocalSafeParser() | ||
|
||
public GremlinGroovysh(final Mediator mediator, final IO io) { | ||
super(io, compilerConfig) | ||
|
@@ -107,41 +107,10 @@ class GremlinGroovysh extends Groovysh { | |
String importsSpec = this.getImportStatements() | ||
|
||
// determine if this script is complete or not - if not it's a multiline script | ||
def status = parser.parse([importsSpec] + current) | ||
def status = remoteParser.parse([importsSpec] + current) | ||
|
||
switch (status.code) { | ||
case ParseCode.COMPLETE: | ||
if (!Boolean.valueOf(getPreference(INTERPRETER_MODE_PREFERENCE_KEY, 'false')) || isTypeOrMethodDeclaration(current)) { | ||
// Evaluate the current buffer w/imports and dummy statement | ||
List buff = [importsSpec] + [ 'true' ] + current | ||
try { | ||
interp.evaluate(buff) | ||
} catch(MultipleCompilationErrorsException t) { | ||
if (isIncompleteCaseOfAntlr4(t)) { | ||
// treat like INCOMPLETE case | ||
buffers.updateSelected(current) | ||
break | ||
} | ||
throw t | ||
} catch (MissingPropertyException mpe) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can't have a |
||
// Ignore any local missing properties since it doesn't affect remote execution. | ||
} | ||
} else { | ||
// Evaluate Buffer wrapped with code storing bounded vars | ||
try { | ||
evaluateWithStoredBoundVars(importsSpec, current) | ||
} catch(MultipleCompilationErrorsException t) { | ||
if (isIncompleteCaseOfAntlr4(t)) { | ||
// treat like INCOMPLETE case | ||
buffers.updateSelected(current) | ||
break | ||
} | ||
throw t | ||
} catch (MissingPropertyException mpe) { | ||
// Ignore any local missing properties since it doesn't affect remote execution. | ||
} | ||
} | ||
|
||
// concat script here because commands don't support multi-line | ||
def script = String.join(Parser.getNEWLINE(), current) | ||
setLastResult(mediator.currentRemote().submit([script])) | ||
|
@@ -175,48 +144,4 @@ class GremlinGroovysh extends Groovysh { | |
|
||
maybeRecordResult(result) | ||
} | ||
|
||
private Object evaluateWithStoredBoundVars(String importsSpec, List<String> current) { | ||
Object result | ||
String variableBlocks = null | ||
// To make groovysh behave more like an interpreter, we need to retrieve all bound | ||
// vars at the end of script execution, and then update them into the groovysh Binding context. | ||
Set<String> boundVars = ScriptVariableAnalyzer.getBoundVars(importsSpec + Parser.NEWLINE + current.join(Parser.NEWLINE), interp.classLoader) | ||
if (boundVars) { | ||
variableBlocks = "$COLLECTED_BOUND_VARS_MAP_VARNAME = new HashMap();" | ||
boundVars.each({ String varname -> | ||
// bound vars can be in global or some local scope. | ||
// We discard locally scoped vars by ignoring MissingPropertyException | ||
variableBlocks += """ | ||
try {$COLLECTED_BOUND_VARS_MAP_VARNAME[\"$varname\"] = $varname; | ||
} catch (MissingPropertyException e){}""" | ||
}) | ||
} | ||
// Evaluate the current buffer w/imports and dummy statement | ||
List<String> buff | ||
if (variableBlocks) { | ||
buff = [importsSpec] + ['try {', 'true'] + current + ['} finally {' + variableBlocks + '}'] | ||
} else { | ||
buff = [importsSpec] + ['true'] + current | ||
} | ||
interp.evaluate(buff) | ||
|
||
if (variableBlocks) { | ||
def boundVarValues = (Map<String, Object>) interp.context.getVariable(COLLECTED_BOUND_VARS_MAP_VARNAME) | ||
boundVarValues.each({ String name, Object value -> interp.context.setVariable(name, value) }) | ||
} | ||
|
||
return result | ||
} | ||
|
||
private boolean isIncompleteCaseOfAntlr4(MultipleCompilationErrorsException t) { | ||
// TODO antlr4 parser errors pop out here - can we rework to be like antlr2? | ||
( | ||
(t.message.contains('Unexpected input: ') || t.message.contains('Unexpected character: ')) && !( | ||
t.message.contains("Unexpected input: '}'") | ||
|| t.message.contains("Unexpected input: ')'") | ||
|| t.message.contains("Unexpected input: ']'") | ||
) | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.tinkerpop.gremlin.console | ||
|
||
import org.apache.groovy.groovysh.ParseCode | ||
import org.apache.groovy.groovysh.ParseStatus | ||
import org.apache.groovy.groovysh.Parsing | ||
import org.codehaus.groovy.control.CompilerConfiguration | ||
import org.codehaus.groovy.control.MultipleCompilationErrorsException | ||
import org.codehaus.groovy.control.messages.Message | ||
import org.codehaus.groovy.control.messages.SyntaxErrorMessage | ||
|
||
/** | ||
* A {@link Parsing} implementation that detects if a Groovy script is complete or incomplete. This implementation uses | ||
* the Groovy compiler to try to compile the script and analyzes any compilation errors to determine if the script is | ||
* incomplete. It does not evaluate the script, which prevents local execution and potential local failure. | ||
*/ | ||
class LocalSafeParser implements Parsing { | ||
|
||
// Try to compile the script | ||
private final CompilerConfiguration config = new CompilerConfiguration() | ||
|
||
// Create a custom GroovyClassLoader that doesn't write class files | ||
private final GroovyClassLoader gcl = new GroovyClassLoader(this.class.classLoader, config) { | ||
@Override | ||
public Class defineClass(String name, byte[] bytes) { | ||
// Skip writing to disk, just define the class in memory | ||
return super.defineClass(name, bytes, 0, bytes.length) | ||
} | ||
} | ||
/** | ||
* Parse the given buffer and determine if it's a complete Groovy script. | ||
* | ||
* @param buffer The list of strings representing the script lines | ||
* @return A ParseStatus indicating if the script is complete, incomplete, or has errors | ||
*/ | ||
@Override | ||
ParseStatus parse(Collection<String> buffer) { | ||
if (!buffer) { | ||
return new ParseStatus(ParseCode.COMPLETE) | ||
} | ||
|
||
// join the buffer lines into a single script | ||
def script = buffer.join('\n') | ||
|
||
try { | ||
// Parse the script without generating .class files | ||
gcl.parseClass(script) | ||
// If we get here, the script compiled successfully | ||
return new ParseStatus(ParseCode.COMPLETE) | ||
} catch (MultipleCompilationErrorsException e) { | ||
// Check if the errors indicate an incomplete script | ||
if (isIncompleteScript(e)) { | ||
return new ParseStatus(ParseCode.INCOMPLETE) | ||
} else { | ||
// Other compilation errors | ||
return new ParseStatus(ParseCode.ERROR, e) | ||
} | ||
} catch (Exception e) { | ||
// Any other exception is considered an error | ||
return new ParseStatus(ParseCode.ERROR, e) | ||
} | ||
} | ||
|
||
/** | ||
* Determine if the compilation errors indicate an incomplete script. | ||
* | ||
* @param e The compilation errors exception | ||
* @return true if the script is incomplete, false otherwise | ||
*/ | ||
private boolean isIncompleteScript(MultipleCompilationErrorsException e) { | ||
def errorCollector = e.errorCollector | ||
|
||
for (Message message : errorCollector.errors) { | ||
if (message instanceof SyntaxErrorMessage) { | ||
def syntaxException = message.cause | ||
def errorMessage = syntaxException.message | ||
|
||
// Check for common indicators of incomplete scripts | ||
if (isUnexpectedEOF(errorMessage) || | ||
isUnclosedBracket(errorMessage) || | ||
isUnclosedString(errorMessage) || | ||
isUnexpectedInput(errorMessage)) { | ||
return true | ||
} | ||
} | ||
} | ||
|
||
return false | ||
} | ||
|
||
/** | ||
* Check if the error message indicates an unexpected end of file. | ||
*/ | ||
private boolean isUnexpectedEOF(String errorMessage) { | ||
errorMessage.contains('unexpected end of file') || | ||
errorMessage.contains('Unexpected EOF') | ||
} | ||
|
||
/** | ||
* Check if the error message indicates an unclosed bracket. | ||
*/ | ||
private boolean isUnclosedBracket(String errorMessage) { | ||
errorMessage.contains("Missing '}'") || | ||
errorMessage.contains("Missing ')'") || | ||
errorMessage.contains("Missing ']'") | ||
} | ||
|
||
/** | ||
* Check if the error message indicates an unclosed string. | ||
*/ | ||
private boolean isUnclosedString(String errorMessage) { | ||
errorMessage.contains('String literal is not terminated') || | ||
errorMessage.contains('Unterminated string literal') | ||
} | ||
|
||
/** | ||
* Check if the error message indicates unexpected input that might suggest | ||
* an incomplete script rather than a syntax error. | ||
*/ | ||
private boolean isUnexpectedInput(String errorMessage) { | ||
// Check for unexpected input but exclude cases where closing brackets are unexpected | ||
// as those typically indicate syntax errors rather than incomplete scripts | ||
(errorMessage.contains('Unexpected input: ') || | ||
errorMessage.contains('Unexpected character: ')) && | ||
!(errorMessage.contains("Unexpected input: '}'") || | ||
errorMessage.contains("Unexpected input: ')'") || | ||
errorMessage.contains("Unexpected input: ']'")) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
interpretorMode
no longer need to be specifically handled with the newRemoteParser
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sense this whole section of code i removed was all unnecessary. Note the
interpreterMode
tests are still passing so I think it's ok.