Community
Participate
Working Groups
Given this class: import java.util.List; public class Test { public static void main(String[] args) { System.out.println("Hello"); } } compile like: $ ecj -proceedOnError -err:+unused Test.java and execute: $ java Test gives Exception in thread "main" java.lang.Error: Unresolved compilation problem: at Test.main(Test.java:4) Normally, the eclipse compiler does a good job in producing usable code even in the presence of errors. In this simple case it fails to do so. This is because an unused import is flagged against the CompilationUnitScope. In that case generateCode assumes that all code within this CU is useless. An immediate soluation would be to make ImportReference extend ReferenceContext and let ProblemReporter.unusedImport(ImportReference) set the import reference as the reference context. This should then be applied to other errors against imports, too. As an alternative trying to generate code for a CUD with errors is probably not a good option.
A solution is to fix it :-). More seriously, if the user wants it tag as an error, then I don't see anything wrong with the actual behavior.
(In reply to comment #1) I guess I should expand this a bit. One reason goes back to the general discussion between static and dynamic languages: people using dynamic languages value the fact that ANY program in that language can be executed, no matter how drafty/buggy etc. - they argue that tests will show and that extremely short edit-run cycles speed up development. I hold that Eclipse implicitely creates a middle ground when it askes at program launch: "project X contains errors, continue launching (yes/no)". With this option we have a slight chance to even convince fans of dynamic languages to use Java (if we desire to do so). Put differently, we can combine the quick edit-run without worrying about irrelevant errors with the benefits of having so many checks in the compiler, which we should all consider before releasing any code - probably before committing to VC. > if the user wants it tag as an error, then I don't see anything > wrong with the actual behavior. You seem to argue that -proceedOnError and launch despite errors are wrong. But the eclipse compiler does try hard to make programs with errors executable, throwing exceptions only when the buggy method is actually invoked. It is not necessary to throw an exception due to a broken import if all methods compiled fine. More specifically, I repeatedly ran into the situation where I want to quickly run two versions of a program, for one version I simply comment-out a few lines. That's "Ctrl-/" and "F11" without even thinking. I'm considerably disrupted by the target program telling me that I have unresolved compilation erros if through that change some imports become unused. I wan't to keep those imports because the next action might well be to re-enable those lines. Summarizing: this behavior disrupts experimental work and it is inconsistent with the rest of the compiler. BTW, my scenario is of course working with the JDT/Core, where it wasn't my decission to make unused import rank as error :)
(In reply to comment #2) > You seem to argue that -proceedOnError and launch despite errors > are wrong. But the eclipse compiler does try hard to make programs > with errors executable, throwing exceptions only when the buggy > method is actually invoked. It is not necessary to throw an > exception due to a broken import if all methods compiled fine. This is a matter of scoping errors. Import errors are considered to be related to the type declaration. When an error occurs within a method, it is scoped to this method. This means that as long as the method is not executed, no exception is thrown. If you have a good suggestion where to report errors related to the type, I can take it. -processOnError simply makes it possible to create what we call "problem" types and "problem" methods. I don't like too much the idea of having an import reference implementing reference context. I'll see if I can better report such errors inside problem types, but I cannot promise anything for now.
(In reply to comment #3) Thanks for considering ... > Import errors are considered to be related to the type declaration. I was going to say that's purely internal, but it actually surfaces wrt @SuppressWarnings. You're right. OTOH, semantically it is scoped to the CUD, which doesn't produce any code at all. I guess that's the reason why different interpretations are possible, right? > If you have a good suggestion where to report errors related to the type, I can > take it. You mean report = throwing an exception? I actually don't think an import error on its own *has to* cause a run-time exception, as long as it doesn't cause further compile errors. Do you see reasons, why an unused import should indeed cause a runtime exception? I would definitely rank it differently than, e.g., a type with unresolvable super. > I don't like too much the idea of having an import reference implementing > reference context. Could you give me a hint, what you dislike about this idea? I was thinking of adding two fields to ImportReferences (sketching from memory): ignoreFurtherInvestigation and declaringCUD. Then only tagAsHavingErrors() and hasError() are answered locally, the other methods from ReferenceContext are delegated to the declaringCUD (resulting in the current behaviour for those). Should I create a patch for experimenting?
Let me give you some background on the problem types and problem methods. The problem types have been added to reduce the number of secondary errors. The problem methods have been added to provide a "runnable" code even if the code contains errors inside methods. The idea is that code with errors should not run when the code that contains error is reached. Basically we don't want people to export code with errors inside products. Unused imports are considered to be warnings by default. In your case you wanted these warnings to be converted to errors. They should be scoped to the compilation unit declaration. But there is not such things as a .class file for a compilation unit. So we "need" to put these errors inside the .class file of the type somehow. If we cannot scope an error to a method, we put it inside the clinit or the default constructor of the type. In this case, we could decide not to report it inside the class file at all. But then this means you can export a type with errors without ever getting an exception at runtime. I don't think we actually need to get an import reference as a reference context in order to fix this issue. We just need to decide what we do for all errors that don't match to a method. Having import reference as a reference context might help to filter these errors from other errors in the type header. Please provide a patch. I'll start from there to see what we can do to filter these errors from the problem type.
(In reply to comment #5) > Let me give you some background on the problem types and problem methods. Thanks, I think we are basically in sync. > The idea is that code with errors should not run when the code that contains > error is reached. OK. > Basically we don't want people to export code with errors inside products. Here I don't follow you. How is an "athrow" inside the byte code connected to exporting? * the export wizards handle the case of compile errors regardless of the actual byte code (I checked jar and plugin exports) * the exception code can never *guarantee* that you'll ever hit the error, so how can the byte code help to *prevent* the export? > Unused imports are considered to be warnings by default. In your case you > wanted these warnings to be converted to errors. They should be scoped to the > compilation unit declaration. But there is not such things as a .class file for > a compilation unit. So we "need" to put these errors inside the .class file of > the type somehow. What do you mean, "\"need\""? > If we cannot scope an error to a method, we put it inside the clinit or the > default constructor of the type. which has the effect that the error is treated as being more severe than an error within a method. > In this case, we could decide not to report it inside the class file at all. > But then this means you can export a type with errors without ever getting an > exception at runtime. I can easily do the same with errors in methods that just happen to never be called. Who says there must be a guarantee that a compiler error must (when?) trigger a runtime exception? The inverse holds: any code that is executed must not have compile errors or throw an exception. But an import statement is never executed, so we're free to handle it either way. > I don't think we actually need to get an import reference as a reference > context in order to fix this issue. We just need to decide what we do for all > errors that don't match to a method. Here's an even bolder idea: all problems of categories like CAT_UNNECESSARY_CODE, CAT_CODE_STYLE ... might avoid setting ignoreFurtherInvestigation regardless of their configured severity. That might give a very clean solution actually. The fact that these problems can be configured as warnings should imply that the compiler is able to proceed with generating sound code. > Having import reference as a reference context might help to filter these > errors from other errors in the type header. > > Please provide a patch. I'll start from there to see what we can do to filter > these errors from the problem type. Coming in a minute.
Created attachment 153947 [details] patch for ImportReference as ReferenceContext A rough sketch of what I initially had in mind. I did go into model, but not yet into codeassist. One nice little CAVEAT: when adding a test to StaticImportTest (as the best approximization for this case) I found this little test called "testONLY_073". You might want to re-enable the other tests of this class, too. ;-P
(In reply to comment #6) > Here I don't follow you. How is an "athrow" inside the byte code connected > to exporting? What I meant is that I don't want users to complain that they could run code that contains errors. In the unused import case, it is a bit different as the unused imports don't prevent the code from compiling and generating the expected bytecodes. > What do you mean, "\"need\""? Until now, the point of problem types and problem methods is that all compile errors (regardless of what they are) are contained into the corresponding problem type. If a problem cannot be matched to a method, it is added to the type clinit. > which has the effect that the error is treated as being more severe than an > error within a method. Right. But this would still be the case for unresolved imports for example. Unresolved imports are supposed to be reported as a compile error as far as I know. > Here's an even bolder idea: all problems of categories like > CAT_UNNECESSARY_CODE, CAT_CODE_STYLE ... might avoid setting > ignoreFurtherInvestigation regardless of their configured severity. > That might give a very clean solution actually. But this might have some effect on the generated code. The patch looks good. Having the import reference as a reference context makes it possible not to flag the unit with errors. Therefore the problem type mecanism is not triggered and it makes the change very localized. Daniel, what do you think about treating some errors differently? The implementation proposed by Stephan is fine and works as expected.
>Daniel, what do you think about treating some errors differently? The >implementation proposed by Stephan is fine and works as expected. I didn't read the whole discussion but I agree that the example from comment 0 should run. The similar thing works using the IDE/UI: if I set unused imports to 'Error' and disable 'Treat errors like fatal compiler error', then the code also executes fine. I would expect that proceedOnError behaves similar to that UI option.
I wonder if the compiler should never treat optional errors as fatal. Right now in the compiler options class, by default all optional errors are treated as fatal. These errors are errors that are not related to the JLS. They are more warnings converted to errors. Just changing this doesn't prevent the code in comment 0 to run and let the problem type/methods mechanism to continue to work when errors are real errors. Stephan, what do you think ? If we do change the default value for the org.eclipse.jdt.internal.compiler.impl.CompilerOptions.treatOptionalErrorAsFatal, then your patch is probably the way to go.
What we really need in this case is a command line option for the batch compiler to treat optional errors as not fatal. Right now there is no way to do this into the batch compiler. We don't actually need to change the default, but we need an option to be able to change this setting. The Eclipse IDE already has such an option. Adding it would just make the batch compiler and the Eclipse IDE in synch.
Created attachment 154493 [details] Proposed fix This adds the new command line option -nonFatalError. Adding this option in the case in comment 0 makes it successful. How do you like it, Stephan?
Why not simply abuse '-proceedOnError' to make optional errors non fatal ?
>Why not simply abuse '-proceedOnError' to make optional errors non fatal ? Right that was what I also had in mind (see comment 9).
(In reply to comment #13) > Why not simply abuse '-proceedOnError' to make optional errors non fatal ? This is still different from the way the IDE is working. In the IDE you can decide to proceedOnError and/or make optional error fatal. I can easily change the behavior of the proceedOnError option in the batch compiler. I thought we wanted to get the same behavior in the batch and in the IDE.
After talking with Philippe, we should introduce a new option that would do -proceedOnError + treat all optional errors as non fatal. -nonFatalError would always be used with -proceedOnError. So it is better to introduce a new option that merges both. We also agreed that the default should be to treat optional errors as non fatal. Right now this is the opposite. All optional errors are always fatal in the batch and the IDE by default. The IDE option is set using the default of the compiler option. As a user, I always want them to be non fatal. I want my code to be executable as much as possible. All this being said, I propose the following action: 1) change the default settings for all optional errors to be non fatal 2) add -proceedOnError:fatal to treat optional errors as fatal 3) -proceedOnError will then consider all optional errors as non fatal In the IDE, there is a change in the default, but I believe this is only a benefit for the end user. I don't see a case where this would be an issue as the user will only get more than what he/she gets right now. Daniel, any comment ?
Fine with me. In the old days Philippe was against being tolerant on those optional errors. We (ZRH lab) always wanted to run in presence of unused imports being reported as errors ;-)
Created attachment 154812 [details] Proposed fix + regression tests This patch doesn't report optional errors as fatal by default anymore. If the old behavior is absolutely desired, then the batch compiler option: -proceedOnError:Fatal should be used instead of -proceedOnError. In the UI, the old behavior can be achieved by setting optional errors to be treated as fatal (UI option in the compiler settings). As you can see, multiple existing tests had to be modified. Of course the documentation should also be updated.
Released for 3.6M5. Regression tests have been updated.
The Javadoc for JavaCore.COMPILER_PB_FATAL_OPTIONAL_ERROR is inconsistent: The main description still contains "Note that by default, errors are fatal, [..]".
Fixed. Replaced with: Note that by default, non optional errors are fatal.
> Note that by default, non optional errors are fatal. That's a bit strange, since non-optional errors *always* fatal and this option is not about non-optional errors. How about this: "Note that by default, optional errors are not fatal. Non-optional errors are always fatal."
Sure, I can change for this. The default value is specified, but I can put this documentation. Released in HEAD.
Verified for 3.6M5 using build I20100122-0800
Verified.
