Skip to content

Commit 8bb3385

Browse files
fix(grammar): resolve JavaCC choice conflicts and gate the build against new ones
Signed-off-by: Andreas Reichel <andreas@manticore-projects.com>
1 parent 894ab4d commit 8bb3385

3 files changed

Lines changed: 176 additions & 7 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,40 @@ env:
2020
JAVA_DISTRIBUTION: 'temurin'
2121

2222
jobs:
23+
grammar_conflicts:
24+
name: Grammar Choice Conflicts
25+
runs-on: ubuntu-latest
26+
steps:
27+
- uses: actions/checkout@v5
28+
with:
29+
fetch-depth: 0
30+
31+
- name: Set up JDK ${{ env.JAVA_VERSION }}
32+
uses: actions/setup-java@v5
33+
with:
34+
java-version: ${{ env.JAVA_VERSION }}
35+
distribution: ${{ env.JAVA_DISTRIBUTION }}
36+
37+
- name: Set up Gradle
38+
uses: gradle/actions/setup-gradle@v4
39+
with:
40+
cache-read-only: ${{ github.ref != 'refs/heads/master' }}
41+
42+
# Re-runs JavaCC on the JJTree output and fails on any "Warning: Choice conflict ...".
43+
# `check` already depends on this task, but a dedicated job gives a fast, separately
44+
# named status check that can be made required in branch protection.
45+
- name: Check for grammar ambiguities
46+
run: ./gradlew checkGrammarAmbiguity --no-daemon
47+
48+
- name: Upload grammar warning report
49+
if: failure()
50+
uses: actions/upload-artifact@v5
51+
with:
52+
name: javacc-warnings
53+
path: build/reports/grammar/
54+
retention-days: 7
55+
if-no-files-found: ignore
56+
2357
gradle_check:
2458
name: Gradle Check (${{ matrix.os }})
2559
runs-on: ${{ matrix.os }}
@@ -182,4 +216,4 @@ jobs:
182216

183217
- name: Deploy to GitHub Pages
184218
id: deployment
185-
uses: actions/deploy-pages@v5
219+
uses: actions/deploy-pages@v5

‎build.gradle‎

Lines changed: 124 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,7 @@ Version {{name}}
506506
{{#commits}}
507507
{{#ifMatches messageTitle "^(?!Merge).*"}}
508508
* **{{{messageTitle}}}**
509-
509+
510510
{{authorName}}, {{commitDate}}
511511
{{/ifMatches}}
512512
{{/commits}}
@@ -717,4 +717,126 @@ jmh {
717717
fork = 3
718718
iterations = 5
719719
timeOnIteration = '1s'
720-
}
720+
}
721+
722+
// ---------------------------------------------------------------------------------------------
723+
// Grammar ambiguity gate
724+
//
725+
// JavaCC reports choice conflicts as "Warning: ..." on stdout and still exits 0, so an ambiguity
726+
// introduced by a PR is completely invisible to `./gradlew check`. The `org.javacc.javacc` plugin
727+
// forwards that output to the Gradle logger, where it cannot be inspected either.
728+
//
729+
// This task therefore re-runs JavaCC on the JJTree output, into a throwaway directory, captures
730+
// stdout+stderr and fails when any warning survives the allow list.
731+
// ---------------------------------------------------------------------------------------------
732+
733+
// Regexes for warnings that are known-benign. Add an entry ONLY together with a comment saying
734+
// why. Never add a "Choice conflict" pattern here — that is the thing this gate exists to catch.
735+
def grammarWarningAllowList = [
736+
// JJTree copies the options block into the .jj; re-passing -OUTPUT_DIRECTORY on the
737+
// command line makes JavaCC complain that it overrides the in-file value.
738+
~/^Warning: .*Command line setting of .* modifies option value in file\./
739+
]
740+
741+
def javaccMainClassOverride = project.findProperty('javaccMainClass')
742+
743+
tasks.register('checkGrammarAmbiguity') {
744+
group = 'verification'
745+
description = 'Fails the build when JavaCC reports choice conflicts (grammar ambiguities).'
746+
747+
dependsOn(compileJavacc)
748+
749+
def jjFile = layout.buildDirectory
750+
.file('generated/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jj')
751+
def scratchDir = layout.buildDirectory.dir('grammar-check')
752+
def reportFile = layout.buildDirectory.file('reports/grammar/javacc-warnings.txt')
753+
def javaccClasspath = configurations.javacc
754+
def allowList = grammarWarningAllowList
755+
def mainClassOverride = javaccMainClassOverride
756+
757+
inputs.file(jjFile)
758+
inputs.files(javaccClasspath)
759+
outputs.file(reportFile)
760+
761+
doLast {
762+
def jj = jjFile.get().asFile
763+
if (!jj.exists()) {
764+
throw new GradleException("Generated grammar not found: ${jj} — did compileJavacc run?")
765+
}
766+
767+
def out = scratchDir.get().asFile
768+
out.deleteDir()
769+
out.mkdirs()
770+
771+
// JavaCC 8 splits into javacc-core / javacc-java; take Main-Class from whichever jar
772+
// declares one, so this keeps working across upgrades of the Manticore JavaCC branch.
773+
def mainClass = mainClassOverride ?: javaccClasspath.files.findResult { f ->
774+
if (!f.name.endsWith('.jar')) {
775+
return null
776+
}
777+
new java.util.jar.JarFile(f).withCloseable { jar ->
778+
jar.manifest?.mainAttributes?.getValue('Main-Class')
779+
}
780+
} ?: 'org.javacc.parser.Main'
781+
782+
def isWindows = System.getProperty('os.name').toLowerCase(Locale.ROOT).contains('win')
783+
def javaExe = new File(new File(System.getProperty('java.home'), 'bin'),
784+
isWindows ? 'java.exe' : 'java')
785+
786+
def proc = new ProcessBuilder(
787+
javaExe.absolutePath,
788+
'-cp', javaccClasspath.asPath,
789+
mainClass.toString(),
790+
"-OUTPUT_DIRECTORY=${out.absolutePath}".toString(),
791+
jj.absolutePath)
792+
.directory(out)
793+
.redirectErrorStream(true)
794+
.start()
795+
796+
def output = proc.inputStream.getText('UTF-8')
797+
def exit = proc.waitFor()
798+
799+
// Warnings are multi-line: a "Warning:" header followed by indented continuation lines.
800+
def warnings = []
801+
def current = null
802+
output.eachLine { line ->
803+
if (line.startsWith('Warning:')) {
804+
current = new StringBuilder(line)
805+
warnings << current
806+
} else if (current != null && line.startsWith(' ')) {
807+
current.append('\n').append(line)
808+
} else {
809+
current = null
810+
}
811+
}
812+
813+
def offending = warnings
814+
.collect { it.toString() }
815+
.findAll { w -> allowList.every { !(w =~ it) } }
816+
817+
def report = reportFile.get().asFile
818+
report.parentFile.mkdirs()
819+
report.setText(offending.isEmpty() ? 'No JavaCC grammar warnings.\n'
820+
: offending.join('\n\n') + '\n', 'UTF-8')
821+
822+
if (exit != 0) {
823+
logger.error(output)
824+
throw new GradleException("JavaCC exited with ${exit} while analysing the grammar.")
825+
}
826+
827+
if (!offending.isEmpty()) {
828+
def conflicts = offending.count { it.contains('Choice conflict') }
829+
logger.error("\n${offending.join('\n\n')}\n")
830+
throw new GradleException(
831+
"${offending.size()} JavaCC grammar warning(s), ${conflicts} of them choice " +
832+
"conflicts. Resolve the ambiguity or add an explicit LOOKAHEAD to the earlier " +
833+
"expansion. Report: ${report}")
834+
}
835+
836+
logger.lifecycle('Grammar is free of JavaCC choice conflicts.')
837+
}
838+
}
839+
840+
tasks.named('check') {
841+
dependsOn('checkGrammarAmbiguity')
842+
}

‎src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4827,7 +4827,11 @@ InsertBulk InsertBulk():
48274827
<K_INSERT> AccessKeyword("BULK") table=Table() { statement.setTable(table); }
48284828
"(" column=InsertBulkColumn() { statement.getColumns().add(column); }
48294829
( "," column=InsertBulkColumn() { statement.getColumns().add(column); } )* ")"
4830-
[ <K_WITH> "(" option=InsertBulkOption() {
4830+
// LOOKAHEAD(2) is required, not cosmetic: Statements() permits two statements to be
4831+
// adjacent without a separator, so FIRST(SingleStatement) -- including <K_WITH> -- is in
4832+
// the FOLLOW set of every statement tail. `WITH (` cannot start a CTE, so two tokens
4833+
// separate the INSERT BULK option list from a following WITH statement.
4834+
[ LOOKAHEAD(2) <K_WITH> "(" option=InsertBulkOption() {
48314835
statement.getOptions().add(option); kinds.add(option.getKind());
48324836
}
48334837
( "," option=InsertBulkOption() {
@@ -4861,6 +4865,10 @@ InsertBulk.Option InsertBulkOption():
48614865
}
48624866
{
48634867
(
4868+
// RelObjectName() below accepts <K_ORDER> as an identifier (isReservedKeywordAsIdentifier()
4869+
// returns true for ORDER unless BY or SIBLINGS follows), so ORDER is a genuine common
4870+
// prefix of the first and the last alternative. The option form always has "(" after it.
4871+
LOOKAHEAD(<K_ORDER> "(")
48644872
<K_ORDER> { option = new InsertBulk.Option(InsertBulk.Option.Kind.ORDER); }
48654873
"(" column=Column() {
48664874
order = new OrderByElement().withExpression(column);
@@ -7290,7 +7298,9 @@ MySqlSelectIntoClause MySqlSelectIntoClause(MySqlSelectIntoClause.Position posit
72907298
|
72917299
LOOKAHEAD({ isMySqlDialect() })
72927300
variable=MySqlIntoVariable() { variables.add(variable); }
7293-
( "," variable=MySqlIntoVariable() { variables.add(variable); } )*
7301+
// Same shape as IntoClause() above: a PlainSelect can be followed by "," (subselect in
7302+
// an expression list), so the repetition needs two tokens to decide.
7303+
( LOOKAHEAD(2) "," variable=MySqlIntoVariable() { variables.add(variable); } )*
72947304
{ intoClause.setType(MySqlSelectIntoClause.Type.VARIABLES); intoClause.setVariables(variables); }
72957305
)
72967306
{
@@ -15055,7 +15065,9 @@ DefaultConstraint DefaultConstraintSpec():
1505515065
[ <K_CONSTRAINT> name=RelObjectName() { constraint.setName(name); } ]
1505615066
<K_DEFAULT> expression=Expression() { constraint.setExpression(expression); }
1505715067
<K_FOR> name=RelObjectName() { constraint.setColumn(new Column(name)); }
15058-
[ <K_WITH> <K_VALUES> { constraint.setWithValues(true); } ]
15068+
// See InsertBulk(): <K_WITH> is in the FOLLOW set of every statement tail. WITH VALUES wins
15069+
// over a hypothetical CTE named VALUES, which is the useful reading here.
15070+
[ LOOKAHEAD(2) <K_WITH> <K_VALUES> { constraint.setWithValues(true); } ]
1505915071
{ return constraint; }
1506015072
}
1506115073

@@ -15556,7 +15568,8 @@ AlterExpressionPrimaryKey CockroachAlterPrimaryKey():
1555615568
}
1555715569
"=" bucketCount=Expression() { action.setBucketCount(bucketCount); } ]
1555815570
]
15559-
[ <K_WITH> options=PostgreSqlIndexOptions() { action.getIndex().setStorageParameters(options); } ]
15571+
// LOOKAHEAD(2) as in the identical constructs at CreateIndex() and AlterExpression().
15572+
[ LOOKAHEAD(2) <K_WITH> options=PostgreSqlIndexOptions() { action.getIndex().setStorageParameters(options); } ]
1556015573
{ return action; }
1556115574
}
1556215575

0 commit comments

Comments
 (0)