Community
Participate
Working Groups
The apitooling.analysis and apitooling.apifreeze tasks should be usable with a bunch of bundles that aren't fully resolved. A component owner or product owner should be able to feed just the bundles he owns through the tooling without having to gather all the dependencies necessary to have a fully resolved Eclipse instance.
How do you resolve a hierarchy if you don't have all the types in the hierarchy ?
I'm not claiming that it is possible to do this. But without this feature it would be really difficult to use the API tooling in our builds. We don't have a fully resolved set of bundles for the previous release (or for the API freeze build) and getting such a set of bundles during the build would be quite difficult. Does the type hierarchy have to be fully resolved? I only care about the API changes in the bundles that are handy. I don't care about the changes in the dependent bundles (at least not for the purposes of this report).
(In reply to comment #2) > Does the type hierarchy have to be fully resolved? I only care about the API > changes in the bundles that are handy. I don't care about the changes in the > dependent bundles (at least not for the purposes of this report). Yes, the type hierarchy has to be fully resolved to be accurate. For example, a method can be deleted from a type, but inherited from a super type. If the super type is not available API tooling sees the method as removed, when it is actually still present (in the super type).
Thanks for the example. I hadn't thought of that. Would it still be useful to support API analysis of unresolved bundles even if the report weren't completely accurate? Would the benefit of getting some API analysis results (vs. having no results because the old bundles can't be resolved) outweigh the cost of some false positives and false negatives? How often would the API analysis results be incorrect, and how often would that cause a team to do the wrong thing? (Those are honest questions. I know what I want the answers to be, but I don't know what the answers actually are.)
(In reply to comment #4) > Thanks for the example. I hadn't thought of that. > > Would it still be useful to support API analysis of unresolved bundles even if > the report weren't completely accurate? Would the benefit of getting some API > analysis results (vs. having no results because the old bundles can't be > resolved) outweigh the cost of some false positives and false negatives? How > often would the API analysis results be incorrect, and how often would that > cause a team to do the wrong thing? > It could be quite useful to see what could be termed the 'raw' (unresolved) references. You could look at it like the difference in getting an AST and an AST with bindings...I think for this route though we would have to make it an option to be set so that users of the task(s) understand that they might not be getting the whole picture. It would probably be good to augment the report to indicate this as well.
Until we try an "unresolved" scan, I don't think we'll know how useful the information might be. There are issues with missing inherited members and with unresolved references (as Michael states). For example, class files refer to methods in terms of declared types. Method invocations are virtual, so referencing Foo.method() does not mean that "Foo" declares the method - it only means the method is available somewhere in its hierarchy. Without a complete hierarhcy, we cannot tell if that method is actually available (unless it is declared in the base class), and what bundle the reference actually resolves to.
In order to accurately/correctly perform API compatibility checks(apitooling.analysis task) and API freeze checks (apitooling.apifreeze), API tooling needs to analyze classes in their complete resolved form with all super types and corresponding type hierarchies in the baseline. In terms of compability and API freeze checks, it's not as accurate to analyze in an unresolved form. A simple shape comparison could be performed in isolation, but proper compariason cannot be performed in the following cases: * bundle splitting and re-exporting. For example, splitting a bundle into core and UI components would case many classes to move to another bundle. Those classes effectively go missing. * Any changes in super interfaces/classes(hierarchies) cannot be properly analyzed * any methods/fields pushed to a super type cannot be properly analyzed
I realize that the results would not be completely accurate if classes can't be completely resolved. But it is just not feasible to for us to have a fully resolved environment with the stuff from the previous release. If the API tooling is unable to do anything useful with an unresolved class hierarchy, then we won't be able to use the API tooling at all. My hope is that an incomplete analysis will still useful and will still catch most of the common errors.
Olivier and I took a look at which compatibility options/checks would work in an environment where not all types can be resolved. Below are the options (listed in the same order as on the preference/properties pages). An 'x' is placed beside items that would break, a '?' is placed beside items that would not work 100%, and those that should work are blank. There are 83 options in total: * 7 options are completely broken * 27 options don't work reliably It's possible that users would want a way to turn off wanrings/problems for checks that are not 100% accurate - or have them in a separate report. The summary is: * compatibility checks for methods, fields, and constructors work OK * compatibility checks for types (classes, interfaces, enums, annotations), are incomplete * API use/rules checks and API leak detection does not work well ================================= API Tools for stand alone bundles ================================= Restrictions: * Cannot detect illegal API use for classes outside the bundle, as we won't have the API description for those classes: - For example, can't know if an interface being implemented is tagged as @noimplement when interface cannot be resolved - API use is pretty much useless in this case, because it's all about inter-bundle use (not use within a bundle) [x] * Implementing an interface tagged @noimplement [x] * Extending a type tagged @noextend [x] * Referencing a field or method tagged @noreference [x] * Instantiating a class tagged @noinstantiate [x] * Overriding a method tagged @nooverride Leaks: * Cannot detect API leaks if we don't have the API descriptions for referenced classes - For example, can't know if a class is internal if it's visibility cannot be resolved in an API description - Will work for local classes being leaked [?] * Extends a non-API class [?] * Implements a non-API interface [?] * Field with non-API type [?] * Method with non-API return type [?] * Method with non-API parameter type Bundle: [ ] * An API type has become non-API [ ] * A type has been removed [x] * An API re-exported type has become non-API [x] * A re-exported type has been removed Class: [?] * A method that needs to be implemented has been added - if an abstract method is pushed down / overridden with new comments, etc., from a super type, won't be able to tell that is was not really added [ ] * Restrictions have been added [ ] * A type parameter has been added [?] * The super interface hierachy has been reduced - could be a case where an explicit super inteface was removed, but a super interface includes it ("A ext B", "C ext A, B" - A is redundant since B extends A, but may not be able to tell) [ ] * the keyword 'abstract' has been added [ ] * The keyword 'final' has been added [ ] * Converted to annotation, enum, or interface [ ] * The visibility has been reduced [?] * A field has been removed - false positive when pushed up [?] * A method has been removed - false positive when pushed up [?] * A constructor has been removed - false positive when pushed up [?] * The superclass hierarchy has been reduced - can't tell unless all super types can be resolved [ ] * A member type has been removed [ ] * A type parameter has been removed Interface: [ ] * A field (interface not tagged as @noimpement) has been added [ ] * A method (interface not taggged as @noimplement) has been added [ ] * Restricitons has been added [?] * A super interface with methods has been added - cannot tell if methods are added unless interface can be resolved [ ] * A type parameter has been added [ ] * Converted to class, enum, or annotation [?] * The super interfaces hierarchy has been reduced - could be a case where an explicit tag was removed, but a super interface includes it ("A ext B", "C ext A, B" - A is redundant since B extends A, but may not be able to tell) [ ] * A type parameter has been removed [?] * A field has been removed - false positive when pushed up [?] * A method has been removed - false positive when pushed up [ ] * A member type has been removed Enum [?] * The super interface hierarchy has been reduced - could be a case where an explicit tag was removed, but a super interface includes it ("A ext B", "C ext A, B" - A is redundant since B extends A, but may not be able to tell) [ ] * Converted to annotation, class, or interface [?] * A field has been removed - false positive when pushed up [?] * A method has been removed - false positive when pushed up [ ] * A member type has been removed Annotation [?] * A method without a default value has been added - could have been pushed down [ ] * Converted to class, enum, or interface [?] * A field has been removed - false positive when pushed up [?] * A method has been removed - false positive when pushed up [ ] * A member type has been removed Field [ ] - A constant value has been added [ ] - A type has been modified [ ] - A constant value has been modified [ ] - The visibility has been reduced [ ] - the keyword 'final' has been removed for static constant [ ] - the keyword 'final' has been added [ ] - The keyword 'static' has been removed [ ] - The keyword 'static' has been added [ ] - The constant value has been removed [ ] - A type argument has been removed Method [ ] - The @nooverride restriction has been added [ ] - A type parameter has been added [ ] - Converted variabe argument to array type [ ] - The visibility has been reduced [ ] - The keyword 'abstract' has been added [ ] - The keyword 'static' has been added [ ] - The keyword 'static' has been removed [ ] - The keyword 'final' has been added (type tagged @noextend) [ ] - The annotation default value has been removed [ ] - A type parameter has been removed Constructor [ ] - A type parameter has been added [ ] - Converted variable argument to array type [ ] - The visibility has been reduced [ ] - A type parameter has been removed Type Parameter - These options could show false positives based on class hierarchy changes [?] - A class bound has been added [?] - An interface bound has been added [?] - A class bound has been modified [?] - An inteface bound has been modified [?] - A class bound has been removed [?] - An interface bound has been removed Version Management [?] - report missing @since tag - hard to tell if something was added or pushed down [ ] - report malformed @since tag [ ] - report invalid @since tag [ ] - report incompatible bundle version
Thank you for the thorough analysis. That helps understand the scope of the issue. When comparing the current build against an earlier build, the current build should be fully resolved. (Otherwise we wouldn't have been able to build it.) It is only the older build that won't be fully resolved. We could, if it would help, have the older build use the same target as the current build, so the older build would usually mostly resolve (even if it wasn't the same version of everything that the old build was originally built against). The first two sections, "Restrictions" and "Leaks", look like they are analyzing just the current build rather than comparing the two builds. Since the current build should be fully resolved, those checks should work fine.
(In reply to comment #10) > The first two sections, "Restrictions" and "Leaks", look like they are > analyzing just the current build rather than comparing the two builds. Since > the current build should be fully resolved, those checks should work fine. Correct, API Use and Leak analysis is performed within a build rather than against a baseline.
I think the common case will be that the current build will be fully resolved. The older build will have the older version of the plugins that we care about and will use the same target as the current build. So it's not like everything will be unresolved. A lot of the question marks have the comment "false positive when pushed up." Since the current build should be fully resolved, the tool should notice that the thing is available in the super class or super interface and not complain. While the results will not be perfect, I think the results will be accurate enough that this is worth doing. There will be some problems that are missed and some false positives, but I think the number will be small enough that people won't stop using the tool entirely.
I think it's crucial the results to be 100% correct in terms of errors reported. False positives are bad. Developers will not trust the scan results and eventually they will lose interest and stop paying attention to them. I think this could be done if when a plugin fails to resolve, the scans that will be affected by that(all item's from Darin's list in comment 9 marked with either "?" or "X") are skipped. That way, all reported errors will be 100% correct(no false positives). And the final report should clearly state that not all scans were ran and lists them. Also, the API analysis task bails out right way when a plugin doesn't have API description. In an email exchange, Michael Rennie noted that most of the scans won't work without API descriptions. Can we see a list of the scans(however small it is) of the scans that will work without API descriptions?
(In reply to comment #13) > I think it's crucial the results to be 100% correct in terms of errors > reported. False positives are bad. Developers will not trust the scan results > and eventually they will lose interest and stop paying attention to them. From the testing we have done so far, false-positives will be inevitable. > I think this could be done if when a plugin fails to resolve, the scans that > will be affected by that(all item's from Darin's list in comment 9 marked with > either "?" or "X") are skipped. That way, all reported errors will be 100% > correct(no false positives). And the final report should clearly state that not > all scans were ran and lists them. That would be one way to handle the case of unknown / unreliable results, we could even detect that the bundle has resolver errors and bail without scanning it at all, and report in the not searched list why it was not searched. > Also, the API analysis task bails out right way when a plugin doesn't have API > description. In an email exchange, Michael Rennie noted that most of the scans > won't work without API descriptions. Can we see a list of the scans(however > small it is) of the scans that will work without API descriptions? Curtis made a wiki found here: http://wiki.eclipse.org/PDE/API_Tools/Tasks that should have that information available. Spending a lot of time trying to get some sane results from supporting unresolved bundles, we sat back and thought "what if there was a simple way to make sure that all required bundles are present when the scan is done?". Then we thought this should be easy to do using target platforms. The only problem is that typically the use of our Ant tasks is done without Eclipse running (headless or otherwise). Peter, if we could support target platforms in our Ant tasks, would that be sufficient to allow you to create analysis / freeze scans? Is the the underlying problem that at scan time all of the bundles that are required to have a resolved profile are distributed; i.e. in a variety of locations, not just one directory (a case which target platforms handle for us)?
Manging all the different problem types and determining which issues could be affected by a unresolved hierarchy is too difficult of a problem to solve in 3.7. For now, we are going to concentrate on improving the reporting of issues. If a bundle is missing part of its hierarchy, we will mark it in the report as such. The bundle will not be analyzed for problems (as any discovered problems could be inaccurate and many existing problems may be missing). The report would be 100% accurate. Users would be encouraged to improve the quality of the report by fixing the missing dependencies in the baseline.
Moving to 3.8 due to lack of resources.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=7a53d56ebca276be7bf4fbe60f195c37e291629a http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=3a7634c1cd2077b625a6942d8bfc401030457b8a With these fixes in master, you can now run the analysis task with unresolved bundles. If resolver errors are found, the bundle will still be analyzed. A warning is added to the index page and to the component page of the HTML report stating that the results may not be accurate. The ways in which results may not be accurate is listed in comment #9. I will copy this information to a wiki page and link to it on the API Tools tasks wiki page. The official task documentation will be updated to note that unresolved bundles will be given a warning. Next step is to look at the freeze task to see how much work is involved to replicate unresolved support there.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=baacb333d2010ddb6c637966618a6e1fc761b877 Adds support for unresolved bundles in freeze task.
(In reply to comment #18) > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > ?id=baacb333d2010ddb6c637966618a6e1fc761b877 > > Adds support for unresolved bundles in freeze task. This caused CHKPII errors. Fixed with http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=896bbb40d2b92f7eedaf13080f282fb3d3270b9b
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=2d214d65227ecae307989a45d85182f82fcb82e7 Adds a property to the tasks so processing of unresolved bundles can be turned off.
Updated task documentation http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=f98e6c1bd39fd1cd2c85b648e08f3bd852a22a14
