problems with annotation compilation when there is an unrelated error in the module

[CeylonMigrationBot]

[@gavinking] See comments by @akberc on ceylon/ceylon-ide-eclipse#578 which is not an IDE problem. @tombentley this one is for you.

[Migrated from ceylon/ceylon-compiler#1757]

[CeylonMigrationBot]

[@tombentley] @akberc, referring back to ceylon/ceylon-ide-eclipse#578 it seems that there merely being errors in the same compilation unit is not enough to trigger the problem. For example a class extending a missing interface, or a class not implementing an inherited formal attribute do not trigger the problem. But a broken annotation class (even if unrelated to the valid annotation classes, and even if only the valid annotation classes are used in the dependent module) is sufficient to trigger the problem.

Do you recall if there were other kinds of broken class which were sufficient the trigger the problem, aside from annotation classes?

[CeylonMigrationBot]

[@tombentley] Let's take the example of a broken class D and a valid function d which instantiates D and returns it (so D appears in its signature).

• We don't generate the class for D, but we do refer to D in the function generated for d.
• When Attr runs is tries to resolve D it tries to complete the symbol, which causes LanguageCompiler to try to parse the source, but CeylonEnter has already been called so we throw a RuntimeException.
• The RuntimeException is caught, and since there are Ceylon typechecker errors we decide there's no bug, so the compiler exits with abnormal status code, but with a Ceylon ERROR state (not BUG).
• Because we silently didn't bother to output a car file, compilation of the modules depending on this module is doomed to fail, which it does with a typechecker error about a missing module.

So fundamentally this is related to error recovery in the transitive case #1637 and #1173.

However, we should not end up hiding the "CeylonEnter has already run" error. In fact I think we should probably be reporting that D is not found in this case. I wonder that @FroMage thinks.

[CeylonMigrationBot]

[@FroMage] Can't we just generate the D class skeleton? Like the one we generate for bootstrapping?

[CeylonMigrationBot]

[@tombentley] Well, that's an idea, but AFAICS we don't actually generate those, we just enter the types into the symbol table, don't we?

[CeylonMigrationBot]

[@FroMage] We only generate them for the bootstrap case. We should just generate them in case of class declaration error too, no? Rather than just skip the whole class.

[CeylonMigrationBot]

[@tombentley] Yeah, I like the theory, I just don't see in the code where/how we do that.

[CeylonMigrationBot]

[@FroMage] In CeylonVisitor? When we detect compilation errors instead of producing nothing?

[CeylonMigrationBot]

[@tombentley] It's certainly possible to generate class and interface declarations, annotated with @CompileTimeError when a declaration has errors. In some particular cases this means compilation continues, a car is produced and dependent code will compile OK, so this would be an improvement.

However, in general generating such things can produce javac errors later during compilation. We've no way to know they're related to the thing we generated (e.g. sometimes errors are attributed to line -1), so we're in the worse position of still not generating a car, but also having added spurious javac errors which look like compiler bugs. We would end up building ever more heuristics into the code generator to try to "fix" broken code so that it was acceptable to javac. We don't want to go there.

We could try to tackle this piecemeal. For instance errors in a class or function parameter can probably be handled robustly by generating a constructor like this:

BrokenParameterList(Object... ignored) {
    throw new UnresolvedCompilationError("whatever error message");
}

That wouldn't work for method parameters though (at least not actual ones), because we can run into problems with not implementing abstract methods in non-abstract classes though. But a piecemeal approach is just that, piecemeal. The original problem would still exists, it would just be slightly less easy to trigger.

I think what we actually need is:

• A boolean in the Ceylon model to mark a Declaration as being broken (currently this information exists only as Errors in the Tree).
• When the code generator encounters a broken Declaration we emit a trivial class, interface or whatever, which exists only for model loading purposes. Importantly if ceylon X depends on broken ceylon Y we guarantee that java X will never depend on java Y$broken.
• Any Declaration that depends on Y is also broken.
• Any Expression that depends on Y has an error added to it (which means it will throw when evaluated).
• Changes to the model loader to mark loaded models as broken when they're annotated with @CompileTimeError

My definition of "depends on" is a little hand-wavy: Containing expressions or statements which use a broken declaration doesn't count. Having the broken declaration anywhere in the dependent declaration (e.g. return type, parameter type, extended, satisfied, enumerated type etc) does count. For example, if I have a broken class A:

• subclass B is marked as broken
• toplevel function f(A) is marked as broken
• toplevel function g(B) is marked as broken

but

• the expression [A(), B()] gets transformed to a tuple construction that throws but it's containing declaration is not marked as being broken.

Thus the brokenness of declarations becomes a transitive thing for declarations, but not for expressions (where it results in an exception at runtime).

[CeylonMigrationBot]

[@FroMage] Well, I wonder what this will do to incremental compilation strategies that compare the source file date with class file date. If we produce a broken class file for users of types with errors, then we fix the error and the incremental checker does not recompile the broken users because its source is older than its class file?

[CeylonMigrationBot]

[@tombentley] Well who said the emitted class file for a broken X had to be X.class? It could be X$broken.class. By definition no other class file will depend on it, so we're got a bit of leeway.

Anyway, the point is that if a type has errors it cannot be used correctly: So those "users" would have to be recompiled. There's no magic way we can compile them correctly so stuff will work once the problem is fixed: It's only a compiler, it can't predict the future.

[CeylonMigrationBot]

[@FroMage] Or we put the list of broken source files in META-INF/broken or something. That would make the ceylon-build script trigger a full rebuild on errors for a naïve implementation which bails out if the file exists, or hell, the compiler itself could auto-add that list of files on the next invocation?

[CeylonMigrationBot]

[@akberc] @tombentley Sorry for late reply - snowed under with day job these days so not able to fire up IDE.

Back then it was primarily annotations, aliases, and changing 'shared' on packages within a module depended on by others, even if the public API of the module did not change. Over time, it was only annotations that were the biggest problem.

I have not read the entire thread, so just speculating : would loose classes for IDE help, instead of modules? This way, intermediate classes would be left in their last knownn stable state and not be recompiled if their dependencies had errors, thus interrupting the cascade. Similar to the 'ambiguous hierarchy' error in the JDT that is one error but does not cascade to dependents. Of course you would not be able to run or export the module with errors.

[CeylonMigrationBot]

[@tombentley] My idea doesn't really work, at least not simply. The basic problem is that we need to generate 'enough' code from a broken declaration in order for the things which depend on it to compile. That means javac has to be happy to compile what it's been presented with, but we also need to be able to load a model from the binary in order to satisfy the ceylon typechecker. For some broken declarations that's not too difficult, but for others it's very complicated, for example:

class BrokenExtends() extends Missing(){}
class Sub() extends BrokenExtends(){}

To compile Sub I need a model for BrokenExtends, but its superclass is missing. So I end up having to pretend its superclass is Object in order to be able to compile Sub.

We end up in the situation where we need to handle each kind of error specially. So we'll end up littering the code generator with logic for specific error cases, and I'm not sure that's a game of whack-a-mole that I want to play.

[CeylonMigrationBot]

[@tombentley] Despite the progress that's been made on #1758 there will still be occasions where we're not sensibly able to generate any .class file for a Ceylon declaration that has errors. (That's because it's only in limited circumstances where we know precisely what the error on the declaration is that we can be sure we can generate something that won't also provoke javac errors at use sites).

So this issue is really about what to do with those Ceylon use sites which we can't compile because they depend on a Ceylon declaration that we've not compiled because it has errors. (I just mean depend on within the declaration; use in the implementation is not a problem, since we just throw). The problem is that this becomes transitive. The idea I tried out involved marking those Ceylon declarations which the compiler could do nothing with, so we could also avoid trying to transform use sites. There are a number of problems with that:

• It requires an unbounded number of visits of the trees as the brokenness gets propagated to dependent declarations.
• The IDE reuses the model, so it would need to go around clearing that flag prior to recompilation.
• In the non-incremental case the typechecker has the model of the broken declaration with which to typecheck the use sites. But in the incremental case it doesn't. This means that you'd get different errors depending on what kind of compilation you were doing. That would be confusing for users.

So I don't really know a satisfactory solution to this problem.

[CeylonMigrationBot]

[@FroMage] Well, this moves to 1.2, right?

[CeylonMigrationBot]

[@tombentley] Yes.

[CeylonMigrationBot]

[@gavinking]

So this issue is really about what to do...

@tombentley if the issue title and description don't remotely describe what the actual problem is, please close it and open a new issue with a better description.