attributing files with syntax errors
Liam Miller-Cushon
cushon at google.com
Fri Aug 21 23:33:01 UTC 2015
A couple of additions:
Changing the default value of shouldStopPolicyIfError to ATTR isn't a good
idea, because the policy was always being checked correctly between parsing
and entering the trees. The bug I noticed only affects the transitive from
enter to attr. I don't see a non-hacky way to preserve the existing default
behaviour.
Here's a better version of the patch that fixes the issue for all of the
policies (not just byfile and bytodo), and changes attribute to use the
same stop-on-error logic as flow:
diff -r eaab8a16dcfb
src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java Thu
Aug 20 11:38:24 2015 -0700
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java Fri
Aug 21 16:30:00 2015 -0700
@@ -860,7 +860,7 @@
case BY_FILE: {
Queue<Queue<Env<AttrContext>>> q = todo.groupByFile();
- while (!q.isEmpty() && !shouldStop(CompileState.ATTR))
{
+ while (!q.isEmpty()) {
generate(desugar(flow(attribute(q.remove()))));
}
}
@@ -1198,7 +1198,7 @@
public Queue<Env<AttrContext>> attribute(Queue<Env<AttrContext>> envs)
{
ListBuffer<Env<AttrContext>> results = new ListBuffer<>();
while (!envs.isEmpty())
- results.append(attribute(envs.remove()));
+ attribute(envs.remove(), results);
return stopIfError(CompileState.ATTR, results);
}
@@ -1206,9 +1206,24 @@
* Attribute a parse tree.
* @return the attributed parse tree
*/
- public Env<AttrContext> attribute(Env<AttrContext> env) {
- if (compileStates.isDone(env, CompileState.ATTR))
- return env;
+ public Queue<Env<AttrContext>> attribute(Env<AttrContext> env) {
+ ListBuffer<Env<AttrContext>> results = new ListBuffer<>();
+ attribute(env, results);
+ return stopIfError(CompileState.ATTR, results);
+ }
+
+ /**
+ * Attribute a parse tree.
+ */
+ protected void attribute(Env<AttrContext> env, Queue<Env<AttrContext>>
results) {
+
+ if (compileStates.isDone(env, CompileState.ATTR)) {
+ results.add(env);
+ return;
+ }
+
+ if (shouldStop(CompileState.ATTR))
+ return;
if (verboseCompilePolicy)
printNote("[attribute " + env.enclClass.sym + "]");
@@ -1237,7 +1252,8 @@
log.useSource(prev);
}
- return env;
+ results.add(env);
+ return;
}
/**
On Thu, Aug 20, 2015 at 6:27 PM, Liam Miller-Cushon <cushon at google.com>
wrote:
> Thanks for taking a look.
>
> On Thu, Aug 20, 2015 at 6:04 PM, Jonathan Gibbons <
> jonathan.gibbons at oracle.com> wrote:
>
>> Twiddling the CompilePolicy is an "internal" knob, controlled by a
>> "hidden" (-XD) option. That being said, my primary concern about changing
>> the bytodo policy is that it is the default policy, so changing it will
>> potentially affect everyone, even those who don't know about the
>> CompilePolicy.
>>
>
> Understood. It's possible that showing errors from attr is actually better
> for most people, since the extra information could be helpful when fixing
> compilation errors. One way to minimize the impact would be to change the
> default shouldStopPolicyIfError mode to attr at the same time the bug was
> fixed, so the default behaviour was unchanged.
>
>
>> And _that_ being said, the original intent of providing the CompilePolicy
>> was to address bugs in which a file had multiple top level classes, and
>> some could have classes generated before errors were detected in other
>> classes in the same file. Hence the byfile policy. But for various
>> reasons, byfile never became the default. I seem to recall it was at least
>> in part due to memory usage, which is less of a concern today than it was
>> back then.
>
>
>> The other area where this might be relevant is annotation processing,
>> where the original code was not so good back then at differentiating
>> different types of errors, and so we wanted compilation to continue more
>> often than we do now. Nowadays, javac has a better concept of "recoverable
>> errors", and so javac is better at not proceeding unnecessarily.
>>
>
> Interesting. The reason we came across this and were experimenting with
> byfile is for error prone. It's occasionally useful to inspect an entire
> compilation unit instead of individual top-level declarations, and with the
> bytodo policy top-level declarations are lowered one at time, so by the
> time everything in a compilation unit has gone through flow some things
> have already been lowered. (I think the plugin api makes it harder to
> observe that by associating events with top-level declarations, and
> discouraging inspecting compilation units.)
>
>
>> So I guess I'd be interested to see the patch, although to prevent
>> unnecessary work up front, it may be worth posting the src/ patch first,
>> with some info on just how many and which tests need fixing. The more
>> tests that need fixup, the more carefully we should review the change in
>> behavior.
>>
>
> Here's the trivial fix I was testing, which just replicates the logic in
> byfile for todo. I realized after doing this that the same issue affects
> all of the other policies, so the check should probably be moved higher up.
>
> diff -r eaab8a16dcfb
> src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java
> ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java
> Thu Aug 20 11:38:24 2015 -0700
> +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java
> Thu Aug 20 18:09:00 2015 -0700
> @@ -867,7 +867,7 @@
> break;
>
> case BY_TODO:
> - while (!todo.isEmpty())
> + while (!todo.isEmpty() && !shouldStop(CompileState.ATTR))
> generate(desugar(flow(attribute(todo.remove()))));
> break;
>
> I counted 37 test failures, excluding two that test compilation policies.
> The list is attached.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20150821/ce1479c7/attachment-0001.html>
More information about the compiler-dev
mailing list