Restore ability to use unannotated stub file to override annotations in bytecode #3599
Conversation
|
The test failure on this PR is interesting. It's not clear to me that the behavior in this PR is wrong, so I'm going to leave the failing test (for now) and instead describe the behavior here. I'd appreciate input from reviewers on which behavior is preferable. The failing test is the one for #2059, which checks that The latter, new error is caused by the change in this PR that attempts to treat an unimported annotation as a fully-qualified name. In this case, what that means is that the stub parser tries to load the class The new error is therefore sensible given what the stub parser is trying to do, but I also think it's somewhat annoying for users. It would require a bit of refactoring to avoid the error, but it's not technically challenging. The question is whether we want to expose to the user that the stub parser tried (and failed) to load the default package's |
framework/src/main/java/org/checkerframework/framework/stub/StubParser.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/stub/StubParser.java
Outdated
Show resolved
Hide resolved
| * The set of annotations found in the stub file. Keys are simple (unqualified) names. (This may | ||
| * be a problem in the unlikely occurrence that a type-checker supports two annotations with the | ||
| * same simple name.) | ||
| * The set of annotations found in the stub file. Keys are names. There are two entries for each |
There was a problem hiding this comment.
In several places you deleted simple, instead, could you please change it to both fully-qualified and simple names. That's important information.
| @@ -173,79 +173,29 @@ private static String formatAnnotations(Collection<? extends Annotation> annos) | |||
| * @return the type formatted to be written to Java source code, followed by a space character | |||
| */ | |||
| private static String formatArrayType(ATypeElement scenelibRep, ArrayType javacRep) { | |||
| int levels = | |||
There was a problem hiding this comment.
The changes in this file should be merged to master in a separate commit. Can you open a separate PR with these changes? I'll merge that PR first, so it's fine to leave these changes in this PR. Also, can you add a test case for this that is unrelated to the other changes in this file?
There was a problem hiding this comment.
The changes in this file should be merged to master in a separate commit
I agree that would be cleaner. The reason I didn't do it is:
can you add a test case for this that is unrelated to the other changes in this file?
I cannot. The error here only affects the order of the parts of the type that are contained in the javac representation. With respect to arrays, that means that the only parts that are incorrectly ordered are the []s - even the inferred annotations were ordered correctly before. The error only shows up when there are printable, but not inferred, annotations - which javac prints using fully-qualified names. So it's not possible to test these changes without the fix to the stub parser, but fixing the stub parser causes the existing tests to fail (because the algorithm is wrong).
There was a problem hiding this comment.
even the inferred annotations were ordered correctly before
What I mean by this is that the sceneLibRep and its associated structures in the old code caused inferred annotations on the inner arrays types to be printed in the correct order, even though the array parts themselves (the []s and any annotations in the javac types) were printed in the wrong order.
There was a problem hiding this comment.
Thanks for the explantation. Now I understand why you can't add a test case (or rather why the existing test case didn't fail until the other bugs where fixed). Could you still open a separate PR without a test case? Then we can just merge that one first, then merge this one.
There was a problem hiding this comment.
#3601 has these changes
…stub-overwrite-bug
Fixes #3597, which was an unintended side-effect of #2871.
The actual fix is making the call to
clearAnnotationsinStubParser#annotateunconditional. Removing that code revealed some other problems, though, which this PR also contains fixes for.The first other problem was that one of the tests for stub-based WPI (
UsesAnonymous.java) failed with an extraoverride.param.invaliderror. Examining the stub file made it clear that the generated stub was (mostly) correct, but that annotations that were actually present in the source code were being ignored (and it seems that the guard for theclearAnnotationscall was a hack to avoid the problem), because they were printed as fully-qualified annotations. The stub parser, however, ignored any annotations that weren't imported directly. I therefore modified the stub parser to also read fully-qualified annotations in stub files.That change fixed the initial failing test, but caused another WPI test case (
MultiDimensionalArrays.java) to fail. Examining the generated stub files for that class made it clear that 3+ dimensional arrays were being printed incorrectly. The test hadn't failed before because the fully-qualified annotations weren't being used. I discovered that our belief about the order in which javac stores arrays was wrong; scene-lib and javac do actually store arrays in the same order. I'm not sure how we made that mistake. That allowed me to remove/simplify a bunch of code inSceneToStubWriter, though I did have to change the specification offormatArrayTypeImpl: it now only prints the "array" part of the type, and relies onformatArrayTypeto print the component type.