Community
Participate
Working Groups
In N20080110-0010, and in general since 3.4M4, I am seeing incorrect deprecation warnings on some annotation instances. The annotation types being warned about are in the same package and are not actually deprecated. Not all instances of the same annotation type in a given compilation unit get the warnings, only some. I will attach a screen capture showing the problem; note that the annotations types @RTVisible, @RTInvisible, and @SimpleAnnotation are showing deprecation warnings (both yellow squiggles and entries in the Problems view), but not all instances of them, only some. If I edit the file (e.g., add whitespace and delete it again), nothing changes. If I then save (and autobuild) the warnings will go away. At this point, the following actions will NOT make the warnings return: - close and reopen the file - save a change to a different unrelated file in the same package - save a change to one of the annotation types involved The following actions WILL make the warnings return: - clean and build of the affected project. - clean and build all projects in the workspace. However, a clean build of all projects *will* make the warnings return. I do not yet have a concise repro case for this. However, it should be possible to see it by checking out the org.eclipse.jdt.apt.tests plug-in from CVS (this will also require org.eclipse.jdt.apt.core, org.eclipse.jdt.core.tests.*, and org.eclipse.test.performance, in order to build). I will try to come up with a more restricted repro case.
Created attachment 86597 [details] Screen shot showing problem
Walter - I'm seeing 8 deprecation warnings in the type AnnotationTest that disappear if I touch the file. Doing a full build brings them back. Is that what you're seeing with HEAD ?
Somehow the package binding of the 3 annotation types is being flagged as deprecated.
So the package question appears to be correctly flagged as deprecated from package-info.java @Deprecated package question; I don't see how an incremental build is going to reproduce this & also keep it flagged as deprecated. So a few questions, is it reasonable to flag a type as deprecated because of its package ? And if so does the package-info.java get 'built' before any type in the package ? Even during an incremental build ?
Re comment #2, yes, that's what I'm seeing. Re comment #4, doh, you're right, package-info.java. So, I retract what I said about the not being deprecated; presumably they should all be deprecated. (I hate package annotations!) Yes, I guess that a type should be deprecated if its package is deprecated; I can't think of any other reason to deprecate a package, other than to get warnings on all its types. The strange thing, unless I'm missing something else obvious, is that only some of the instances of that annotation type in that file are flagged as deprecated, and then only in incremental build. Did I mention how I feel about package annotations?
1. The incremental v full build disparities in warnings issue aside: are these warnings even required/meaningful/useful ? The package in question namely ``question'' is being deprecated and the warnings (when they are produced) are about the components that *constitute* the package (and not for some external client entity that imports it which would make sense) Per JLS 3.0, section 9.6.1.6, A program element annotated @Deprecated is one that programmers are discouraged from using, typically because it is dangerous, or because a better alternative exists. A Java compiler must produce a warning when a deprecated type, method, field, or constructor is used (overridden, invoked, or referenced by name) unless: • The use is within an entity that itself is is annotated with the annotation @Deprecated; So is, must + unless == must not ? || must + unless == need not ? 2. So the real question is, for references outside the constituents of the package, is there a consistent, predictable and correct warning behavior regardless of incremental v full build ? This needs to be checked.
*** Bug 99638 has been marked as a duplicate of this bug. ***
From bug 99638, javac's bug is http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6492694 I've tried the latest javac 7 version & deprecated packages still have no affect in javac inside or outside of their package. Do we want to support deprecated packages at all ? I tried : javac -Xlint:deprecation question\package-info.java question\A.java question\B.java other\Z.java file: question/package-info.java @Deprecated package question; file: question/A.java package question; public class A { B b; } file: question/B.java package question; public class B { A a; } file: other/Z.java package other; import question.*; class Z extends question.A { B b; }
Walter, how load would you scream if we stopped tagging packages as deprecated ?
If only I could type... How loud would you scream if we stopped tagging packages as deprecated?
You mean, if you stopped issuing a warning on deprecated packages? I don't have any problem with that at all. My strictly personal opinion is that package-info.java is a crock. If you're talking about hiding the 'deprecated' attribute on a package object altogether - so that there's no way for APT to determine whether a given package declaration has the @Deprecated annotation - I doubt it is a user issue but it might conceivably cause the compiler to fail Java compliance tests.
Yes - it was the second one. As far as the compiler & likely most users are concerned, the notion of a package being deprecated is not interesting. But if APT tests care, then we're stuck...
(In reply to comment #12) > But if APT tests care, then we're stuck... It wouldn't be APT tests per se - if any APT tests break, we can change them. I'm more worried about Sun JCK tests. No timely way to find out whether those break, though. The JLS does explicitly mention (7.4.1.1) the possibility of package annotations, so it is reasonable to expect that the compliance tests cover them. Whether they touch on @Deprecated in particular is unknown, but I think that if we ignore package annotations altogether we'll probably be in trouble.
Perhaps by coincidence, bug 258906 was just reported, which appears to be the same problem, package-level annotations not being visible to a Java 6 processor. I haven't dug into that one yet but this is an indication that contrary to my hopes, people actually are trying to use this feature in the field.
Srikanth - I'm going to take this one back since its going to be tricky to get the resolution order correct for incremental builds.
(In reply to comment #15) > Srikanth - I'm going to take this one back since its going to be tricky to get > the resolution order correct for incremental builds. Hello Kent, I am happy working on it - as long as it is recognized to be a tricky issue and as such one that would take some time for a newcomer to fix. I am happy not working on it also :), so either way is good.
Srikanth - give it a try. I suggest you start with bug 258906 - its easier. I have a patch for this case & bug 258906 that seems to work so ask for hints if you want. Not much will happen around here for the next 2 weeks so we can 'fix' it in a week or two.
Created attachment 121836 [details] Preliminary patch Kent, take a look and let me know if you see any major problems. This patch addresses most of the issues, but still needs more testing. The one known glitch is : When the name/lookup environment instantiated is one that is based on the Java model (such as the Cancellable/Searchable env), there is a problem is restoring package level deprecations. This is because the Java model does not store the package-info synthetic type. I think it is really a bad idea that package-info type was hidden from the Java model. I can't think of any good reason for this. I'll continue to investigate this and see how this can be resolved. (unless you have solution already) This patch in particular ensures that, - Deprecated warnings are not issued when already in deprecated context - Incremental build of package-info.java by changing and saving package-info.java file triggers a build of other elements in the package if the said changes involve annotation changes. - In full build mode, package level deprecation details are available during resolution of any type (even for a type that preceded package-info.java in the compile queue)
Created attachment 121875 [details] Scope change Instead of your change to Scope try this one instead (it requires no changes to MethodBinding) The ClassFileReader change is covered by bug 149768 - double check that patch please. Also take a look at CompilationUnitDeclaration where the package annotations are currently resolved to see if that is still necessary.
(In reply to comment #18) > I think it is really a bad idea that package-info type was hidden from the > Java model. I can't think of any good reason for this. I'll continue to > investigate this and see how this can be resolved. (unless you have solution > already) The Java model only shows what's in the source. Since the package-info.java file doesn't contain any type, the fake package-info type cannot appear as a child of the package fragment. Similarly, if an interface defines a method "void myMethod();" the flags (IMember#getFlags()) don't include the 'public' flag. That said, we could still store the deprecation on the IPackageFragment (by making it an IAnnotatable for example.)
Created attachment 121950 [details] Patch with earlier review suggestions incorporated Kent, please take a look again. This patch addresses the previous comments. It also restores package deprecation status properly even when org.eclipse.jdt.internal.compiler.lookup.PackageBinding.getType(char[]) fails to find package-info.java due to its absence in the model. I am yet to run the automated tests fully. I am posting this patch in advance for fear that my laptop may die on me anytime, I met with the blue screen more than a couple of times in the last few hours - if tests finish ok, I'll update status here. I also haven't created a test yet, did you happen to write any ?
(In reply to comment #20) > (In reply to comment #18) > > I think it is really a bad idea that package-info type was hidden from the > > Java model. I can't think of any good reason for this. I'll continue to > > investigate this and see how this can be resolved. (unless you have solution > > already) > The Java model only shows what's in the source. Since the package-info.java > file doesn't contain any type, the fake package-info type cannot appear as a > child of the package fragment. Similarly, if an interface defines a method > "void myMethod();" the flags (IMember#getFlags()) don't include the 'public' > flag. > That said, we could still store the deprecation on the IPackageFragment (by > making it an IAnnotatable for example.) I see what you mean - this issue has reared its head in some manner, form or fashion (either complicating the fix or writing the test) in the three or four defects I worked on this area - so you can see where my comment is coming from! I wonder if it would have been better for the model to have still stored package-info and supplied some filters which clients can engage to hide them.
Tests finished OK.
Created attachment 122006 [details] Proposed patch This is the patch that works for me. Does it fail some of your tests ? Is all of the extra code for the JavaModel case in PackageBinding.getAnnotationTagBits() necessary ? I thought PackageElementImpl.getAnnotationBindings() handled finding the annotations for a package in the DOM. Also, do you have a case that requires the additional QualifiedReference in CompilationUnitScope ? I do not need an additional reference to get incremental builds to work. For testing - I'm using Walter's case & checking incremental vs. full build scenarios. And what does setting 'declaration.javadoc = this.referenceContext.javadoc' in the CompilationUnitScope do ? Who needs it ?
(In reply to comment #24) > Created an attachment (id=122006) [details] > Proposed patch > This is the patch that works for me. Does it fail some of your tests ? Yes, it does. > For testing - I'm using Walter's case & checking incremental vs. full build > scenarios. Per steps in comment #0 in bug #258906 import the `testprocessor' project into your workspace and import the project 'test' into the PDE workspace you run from your top level eclipse instance. 1. Full build of 'test' should not throw any errors since that bug #258906 is closed. 2. Now edit package-info.java and comment out the line @javax.xml.bind.annotation.XmlSchema (namespace = "test") and save it. 3. Autobuild triggers. You should really get an error "XML namespace must be given in @XmlType or @XmlSchema", but you DON'T. 4. Clean and trigger full build - you get the error. 5. Edit package-info.java and undo (2) and save. Error does not go away. 6. Clean and full build - error goes away. > Also, do you have a case that requires the additional QualifiedReference in > CompilationUnitScope ? I do not need an additional reference to get incremental > builds to work. As my comment in the code explains, if you introduce material changes to package-info.java and save it, thereby triggering in an incremental compile of it, there is nothing in HEAD that ensures that elements in the package get rebuilt. Creating the additional QualifiedReference in CompilationUnitScope achieves this. > And what does setting 'declaration.javadoc = this.referenceContext.javadoc' in > the CompilationUnitScope do ? Who needs it ? This is some `drive-by-bug-shooting'. This change is unrelated to the current defect, nor does it actually cause a problem today (the way the downstream code works). It is just a dormant bug in that, the concerned code on HEAD leaves the AST in an improper state. Compare the synthetic type creation code in CompilationUnitScope.buildTypeBindings with the code in Parser.consumeEmptyInternalCompilationUnit for example. > Is all of the extra code for the JavaModel case in > PackageBinding.getAnnotationTagBits() necessary ? I thought > PackageElementImpl.getAnnotationBindings() handled finding the annotations for > a package in the DOM. Please apply the soon to be attached tracedump patch on top of (head + your patch) and try the following steps. 1. Launch a PDE instance, import eclipse.org.jdt.apt.tests into it along with needed projects mentioned in comment #0. 2. Clean and trigger full build of eclipse.org.jdt.apt.tests. Plya around a bit by changing notypes.package-info.java a bit with and without immediate save. You should see the following on the console: BAD: question.SimpleAnnotaion is NOT viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! GOOD: question.SimpleAnnotaion is viewed as deprecated ! BAD: question.SimpleAnnotaion is NOT viewed as deprecated ! So, on few of the occasions we are wrong. My changes in PackageBinding.java addresses these cases. It should be mentioned that so far as I have seen, the BAD cases occur only in Daemon Thread [org.eclipse.jdt.internal.ui.text.JavaReconciler] (Suspended (breakpoint at line 473 in ASTNode)) QualifiedTypeReference(ASTNode).isTypeUseDeprecated(TypeBinding, Scope) line: 473 QualifiedTypeReference(TypeReference).internalResolveType(Scope) line: 150 QualifiedTypeReference(TypeReference).resolveType(BlockScope, boolean) line: 197 QualifiedTypeReference(TypeReference).resolveType(BlockScope) line: 193 SingleMemberAnnotation(Annotation).resolveType(BlockScope) line: 233 ASTNode.resolveAnnotations(BlockScope, Annotation[], Binding) line: 614 SourceTypeBinding.getAnnotationTagBits() line: 691 SourceTypeBinding.faultInTypesForFieldsAndMethods() line: 588 CompilationUnitScope.faultInTypes() line: 450 CompilationUnitProblemFinder(Compiler).resolve(CompilationUnitDeclaration, ICompilationUnit, boolean, boolean, boolean) line: 847 CompilationUnitProblemFinder(Compiler).resolve(ICompilationUnit, boolean, boolean, boolean) line: 899 CompilationUnitProblemFinder.process(CompilationUnit, SourceElementParser, WorkingCopyOwner, HashMap, boolean, int, IProgressMonitor) line: 182 CompilationUnitProblemFinder.process(CompilationUnit, WorkingCopyOwner, HashMap, boolean, int, IProgressMonitor) line: 243 ReconcileWorkingCopyOperation.makeConsistent(CompilationUnit) line: 190 ReconcileWorkingCopyOperation.executeOperation() line: 89 ReconcileWorkingCopyOperation(JavaModelOperation).run(IProgressMonitor) line: 721 ReconcileWorkingCopyOperation(JavaModelOperation).runOperation(IProgressMonitor) line: 781 CompilationUnit.reconcile(int, int, WorkingCopyOwner, IProgressMonitor) line: 1242 JavaReconcilingStrategy.reconcile(ICompilationUnit, boolean) line: 124 JavaReconcilingStrategy.access$0(JavaReconcilingStrategy, ICompilationUnit, boolean) line: 108 JavaReconcilingStrategy$1.run() line: 89 SafeRunner.run(ISafeRunnable) line: 37 JavaReconcilingStrategy.reconcile(boolean) line: 87 JavaReconcilingStrategy.reconcile(IRegion) line: 149 JavaCompositeReconcilingStrategy(CompositeReconcilingStrategy).reconcile(IRegion) line: 86 JavaCompositeReconcilingStrategy.reconcile(IRegion) line: 102 JavaReconciler(MonoReconciler).process(DirtyRegion) line: 77 AbstractReconciler$BackgroundThread.run() line: 206 and never in Daemon Thread [Compiler Processing Task] (Suspended (breakpoint at line 478 in ASTNode)) SingleTypeReference(ASTNode).isTypeUseDeprecated(TypeBinding, Scope) line: 478 SingleTypeReference(TypeReference).internalResolveType(Scope) line: 150 SingleTypeReference(TypeReference).resolveType(BlockScope, boolean) line: 197 SourceTypeBinding.resolveTypesFor(MethodBinding) line: 1395 SourceTypeBinding.methods() line: 1087 SourceTypeBinding.faultInTypesForFieldsAndMethods() line: 593 CompilationUnitScope.faultInTypes() line: 450 Compiler.process(CompilationUnitDeclaration, int) line: 731 ProcessTaskManager.run() line: 137 Thread.run() line: 735 C:\IBMJava60\jre\bin\javaw.exe (Jan 9, 2009 10:35:54 AM) I think this is due to the name environments being different, one based on model and the other file system. I don't have a test case that actually shows an end user problem due to this. I don't think we anyway guarantee that on the fly reconcile operation would catch all problems. So may be we are ok leaving out these elaborate changes. Please use your judegement. In the event of deleting these changes, leaving the comment intact with an added reference to this defect may make sense ?
Created attachment 122065 [details] Trace dumper patch This shows potential bug in org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding.isViewedAsDeprecated() when invoked by the Java Reconciler.
(In reply to comment #22) > I wonder if it would have been better for the model to have still stored > package-info and supplied some filters which clients can engage to hide them. That might work as well. However if you surface it to the clients with a flag, I think the default should be to hide the fake type so that existing clients are not broken (i.e. a client that retrieves the source range of the type could be broken if this source range is (-1, -1). )
Created attachment 122138 [details] Proposed patch Good work - but instead of adding an additional reference to every single compilation unit for the package-info type, which may or may not exist, I would prefer to detect this case in the incremental builder. As for the extra code in PackageBinding, I'm inclined to leave it out for now, but we should talk about it on Monday.
Created attachment 122149 [details] Proposed patch Actually I prefer this one instead
(In reply to comment #29) > Created an attachment (id=122149) [details] > Proposed patch > Actually I prefer this one instead PackageBinding.java has some System.out.print statements left in. It may be useful to add a comment to PackageBinding.getAnnotationTagBits() to the effect that it doesn't retrieve the deprecation annotations properly if the INameEnvironment is based on Java model.
Created attachment 122306 [details] Added test
Fix and test released for 3.5M5
Verified for 3.5M5 using I20090126-1300
