Community
Participate
Working Groups
With the introduction of restricted keywords (see bug 488541) scanning is no longer possible without also parsing. For that reason the interface IScanner is not useful in Java 9 (unless clients ensure to use it only for ordinary compilation units). This should be documented both in IScanner and ToolFactory. Since JDT/Core cannot detect when this API is invoked with the source of module-info.java (because the filename is not passed in this scenario), we cannot even issue / log any warning when a module descriptor is passed. Since this means we cannot distinguish "good" from "bogus" use of the API, it may be best to complete deprecate it. The only way to "fix" the API would be by introducing additional methods like setIsModuleDescriptor(boolean) and *require* that clients call it prior to scanning. Obviously, this would be a breaking change, too. So I'm not sure it's worth it. Actually, setIsModuleDescriptor() wouldn't be enough as long as setSource() accepts any snippet of code. If module-info.java is passed, the source *must* include the opening "module" keyword, otherwise classification of restricted keywords is not possible.
Proposed text for the start of the javadoc of IScanner, similar blurp to be added to all ToolFactory.createScanner() methods: ------------- <strong>Caveat:</strong> With the introduction of "restricted keywords" in Java 9 it is impossible to classify a token without the help of a parser. For that reason, this interface is not suitable for scanning a modular compilation unit ("module-info.java"). It is the client's responsibility to pass only source from ordinary compilation units. For lack of a file name the scanner cannot check this. ------------- Thoughts? Should this functionality be deprecated, to make the caveat more visible? As a compromise we could deprecate only IScanner.getNextToken() which is the only method actually promising to classify tokens (which it no longer can in a reliable way). In that case I would attach the "caveat" text to - IScanner.getNextToken() - and similarly to ToolFactory.createScanner() as an "early warning" At the least, the same blurp should go into the readme / migration guide.
Proposed deprecation notice of IScanner.getToken(): @deprecated with the introduction of "restricted keywords" in Java 9, classification of tokens cannot reliably be done by a scanner, and hence the only real solution is to use a parser instead (see also the caveat at the top of the interface documentation).
New Gerrit change created: https://git.eclipse.org/r/103734
Some figures from my JDT workspace: Method IScanner.getNextToken() is referenced 66 times (not counting tests). Of these, 40 references are in JDT/UI (only few in /Debug). Of those 40, 14 locations are interested in distinguishing identifiers vs. keywords. These locations are potentially affected by the restricted keyword problem, if this code could be reached with a modular compilation unit (which I haven't checked). I think fairness demands to somehow warn adopters that existing use of JDT API may not work as expected in Java 9. For that reason deprecating sounds like a suitable means, but I'm not sure if we'd be unnecessarily spamming adopters. From the above numbers I think @SW("deprecation") is a suitable means to acknowledge this new problem. If that's considered too harsh I see three alternatives: (1) revert @Deprecated and hope that a notice in the readme suffices to alert adopters (2) or add a new non-deprecated API, one of: (a) getNextOrdinaryToken() (using the same implementation internally) (b) a subtype IOrdinaryScanner with corresponding factory methods in ToolFactory In this type, #getNextToken() will not be deprecated Either sub-option allows adopters to acknowledge the new problem by simply migrating to an equivalent API, ensuring they know that this is not applicable to modular compilation units. IF NEEDED, we could then (later) add the *Modular* variants which will have restricted keywords enabled - at best effort approach. (3) instead of deprecating #getNextToken(), what about deprecating the constant ITerminalSymbols#TokenNameIdentifier? Numbers in my workspace: 85 references not counting tests. No reference in JDT/Debug Merely 8 references in JDT/UI. This looks much more precise to me. (JDT/Core could use an internal alias after checking if each reference is safe) Final remark: Future Java extensions will aggravate this issue. Sweeping the current problem under the rag will create much bigger surprise when the rag can no longer cover the problem in the future. Comments? Preferences?
I think more important than moving the target back and forth is: could someone please comment on the options I outlined (and perhaps others I didn't think of)?
New Gerrit change created: https://git.eclipse.org/r/117971
(In reply to Eclipse Genie from comment #6) > New Gerrit change created: https://git.eclipse.org/r/117971 Consists of: - Informal caveat at the top of IScanner - @Deprecated at ITerminalSymbols#TokenNameIdentifier
Gerrit change https://git.eclipse.org/r/117971 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=fe21ba1003ebd5cc75c5cefdcd63ad8835d40f97
New Gerrit change created: https://git.eclipse.org/r/117979
(In reply to Eclipse Genie from comment #8) > Gerrit change https://git.eclipse.org/r/117971 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=fe21ba1003ebd5cc75c5cefdcd63ad8835d40f97 JDT/Core changes released for 4.8 M6 (In reply to Eclipse Genie from comment #9) > New Gerrit change created: https://git.eclipse.org/r/117979 Some follow-up changes in JDT/UI: suppress some warnings where it seems safe. Comment other locations.
Gerrit change https://git.eclipse.org/r/117979 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d64ed58ebbe7f94e8d2bf38d698b715027951334
No more changes planned as of now.
(In reply to Stephan Herrmann from comment #1) > At the least, the same blurp should go into the readme / migration guide. Has this been done? See also bug 534459.
(In reply to Dani Megert from comment #13) > (In reply to Stephan Herrmann from comment #1) > > At the least, the same blurp should go into the readme / migration guide. > > Has this been done? See also bug 534459. Will add it now.
(In reply to Jay Arthanareeswaran from comment #14) > Will add it now. Done. The documentation can be seen here: https://www.eclipse.org/eclipse/development/readme_eclipse_4.8.php#mozTocId521249
