Community
Participate
Working Groups
- Create a module-info.java file as: module Try1 { exports p; requires java.base; } - Press Ctrl+Shift+F in the file. => The file is not formatted. The kind used to format is: CodeFormatter.K_COMPILATION_UNIT
@Mateusz: Could you please take this up as a priority as this is targetted for Beta Java 9?
My understanding is we need the DOM nodes (at least be able to understand what nodes represent the new program elements in module-info) for the formatter to work. Of course, Mateusz is the right person to confirm.
I should find some time to look at it this weekend or next week. Yes, I imagine JAVA9 should have new DOM nodes and corresponding methods in ASTVisitor for them - if that's the case, the formatter implementation should be easy.
(In reply to Mateusz Matela from comment #3) > Yes, I imagine JAVA9 should have new DOM nodes and corresponding methods in > ASTVisitor for them - if that's the case, the formatter implementation > should be easy. Not yet - was waiting for a client requirement and now we have this requirement - will address this bug 496123
(In reply to Manoj Palat from comment #4) > (In reply to Mateusz Matela from comment #3) > > > Yes, I imagine JAVA9 should have new DOM nodes and corresponding methods in > > ASTVisitor for them - if that's the case, the formatter implementation > > should be easy. > > Not yet - was waiting for a client requirement and now we have this > requirement - will address this bug 496123 @Mateusz: the dom ast for module is in. let know if formatter requires any additional support from dom.
I need help with using the parser - I can't get it to work. I set the source level to 9 and pass in a sample code. In DefaultCodeFormatter.parseSourceCode(ASTParser, int, boolean) I get an ASTnode with no elements and with the following problems: [Pb(204) Syntax error on token "module", interface expected, Pb(204) Syntax error on token ".", , expected] I see it working in ASTConverter9Test. Maybe the name of the compilation unit matters? But the formatter doesn't know it, it only has the source code... Another topic: do you think it's worth adding new settings for modules, about indentation, brace positions, and blank lines? Maybe we can simply use corresponding options for classes, it's hard to imagine someone needing a different format for modules. New line wrapping options seem more necessary, but is one general option for module declarations enough, or rather two separate options for "exports" and "provides"?
(In reply to Mateusz Matela from comment #6) > I need help with using the parser - I can't get it to work. I set the source > level to 9 and pass in a sample code. In > DefaultCodeFormatter.parseSourceCode(ASTParser, int, boolean) I get an > ASTnode with no elements and with the following problems: > > [Pb(204) Syntax error on token "module", interface expected, > Pb(204) Syntax error on token ".", , expected] > > I see it working in ASTConverter9Test. Maybe the name of the compilation > unit matters? But the formatter doesn't know it, it only has the source > code... Can you send me/attach the project in the bug which exhibits the issue. I'll take a look > > Another topic: do you think it's worth adding new settings for modules, > about indentation, brace positions, and blank lines? Maybe we can simply use > corresponding options for classes, it's hard to imagine someone needing a > different format for modules. > > New line wrapping options seem more necessary, but is one general option for > module declarations enough, or rather two separate options for "exports" and > "provides"? Unless there is an explicit request from somebody, I think its not worth to have separate section for modules as such - can reuse class. We can have a general option for module statements rather than each type of module statement.
(In reply to Manoj Palat from comment #7) > (In reply to Mateusz Matela from comment #6) > > I need help with using the parser - I can't get it to work. I set the source > > level to 9 and pass in a sample code. In > > DefaultCodeFormatter.parseSourceCode(ASTParser, int, boolean) I get an > > ASTnode with no elements and with the following problems: > > > > [Pb(204) Syntax error on token "module", interface expected, > > Pb(204) Syntax error on token ".", , expected] > > > > I see it working in ASTConverter9Test. Maybe the name of the compilation > > unit matters? But the formatter doesn't know it, it only has the source > > code... > > Can you send me/attach the project in the bug which exhibits the issue. I'll > take a look > reproduced.. will check and let know
(In reply to Manoj Palat from comment #8) > (In reply to Manoj Palat from comment #7) > > (In reply to Mateusz Matela from comment #6) > > > I need help with using the parser - I can't get it to work. I set the source > > > level to 9 and pass in a sample code. In > > > DefaultCodeFormatter.parseSourceCode(ASTParser, int, boolean) I get an > > > ASTnode with no elements and with the following problems: > > > > > > [Pb(204) Syntax error on token "module", interface expected, > > > Pb(204) Syntax error on token ".", , expected] > > > > > > I see it working in ASTConverter9Test. Maybe the name of the compilation > > > unit matters? But the formatter doesn't know it, it only has the source > > > code... > > > > Can you send me/attach the project in the bug which exhibits the issue. I'll > > take a look > > > reproduced.. will check and let know @Mateusz: There are two issues here - one is that of setting compliance level to 9 and the other the parser needs to recognize that it is parsing a module. The first issue has been solved in other test cases such as FormatterRegressionTests#test504 and others by setting and resetting the JavaCore options. However, the second one is a little tricky since the parser should know that this is parsing a module-info which currently is checked via Scanner#isInModuleDeclaration() which in turn calls CUD#isModuleInfo() - this currently is only a simple module-info filename check. So until we device more "sophisticated" solution, I would suggest to write a test case in a file named "module-info.java" along the lines of ASTConverter9Test#testBug496123_0001(). Please note to create a copy of Converter9 directory and name it Formatter9 so that the project settings file will also be copied automatically and the parser compliance settings will be inherited from this project thus making it 9 (which essentially means that the first problem will be solved). Then create a module-info.java file as done in that test case to get the compilation unit with the module. Hope this helps!
OK, but what do we do about the fact that formatter doesn't actually know the compilation unit's name when it gets the source code to process? Option 1: Extend the formatter's API so that the user can (must?) provide the file name. It doesn't seem reasonable to make such a change just to allow formatting module-info. Option 2: When the parsing fails (i.e. there are any problems reported and the CompilationUnit has no type declarations), the formatter tries again and pretends that the file name is module-info.java. If the second parsing returns a CompilationUnit containing a module declaration, it's good to go. This solution seems pretty dirty to me, but it should work. Option 3: Would you consider changing the parser so it doesn't need to know the file name? I thought it could work this way because there's no problem with package-info.java, but maybe these are not really similar cases. Would this be a possible more "sophisticated" solution you mentioned?
(In reply to Mateusz Matela from comment #10) > OK, but what do we do about the fact that formatter doesn't actually know > the compilation unit's name when it gets the source code to process? > > Option 1: Extend the formatter's API so that the user can (must?) provide > the file name. It doesn't seem reasonable to make such a change just to > allow formatting module-info. > > Option 2: When the parsing fails (i.e. there are any problems reported and > the CompilationUnit has no type declarations), the formatter tries again and > pretends that the file name is module-info.java. If the second parsing > returns a CompilationUnit containing a module declaration, it's good to go. > This solution seems pretty dirty to me, but it should work. > > Option 3: Would you consider changing the parser so it doesn't need to know > the file name? I thought it could work this way because there's no problem > with package-info.java, but maybe these are not really similar cases. Would > this be a possible more "sophisticated" solution you mentioned? @Mateusz: I think a more appropriate solution would be a combination of option 1 and 3 ie Let us extend the formatter's API as mentioned in option 1, but let us not bind it to a filename. We may go away from the file name check and may replace with something else - sophisticated in the sense that that does not depend on the filename but might depend on parsing (most likely) and/or existing at a particular directory - at this point we haven't fixed yet. In short, let us add a lambda expression as the additional argument which for now takes in the full path of the filename and returns a boolean - [true indicating parse for module-info] - This gives us the freedom to use the parameter in a way we want later. interface I { boolean isModule(String s); }
(In reply to Manoj Palat from comment #11) > fixed yet. In short, let us add a lambda expression as the additional > argument which for now takes in the full path of the filename and returns a > boolean - [true indicating parse for module-info] - This gives us the > freedom to use the parameter in a way we want later. > Updating the result of an offline discussion with Markus: At this point, we can keep the new API very simple - just another boolean which flags whether the code is in module-info or not. [ The main reasons for avoiding lambda expression are a) the lambda expression would make it very flexible which may not be what we want in an API and b) we don't expect the name of the module-info.java to change). Hence please create a new API with one additional boolean parameter.
Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=49e9d0cec84062b8f937bcce9ab15ec576e3a013 I've added CodeFormatter.K_MODULE_INFO constant as a new kind for mule info files. It seemed more natural than adding a new property or method. One more thing that could have a new setting is spaces around commas in module statements. For now I've used the same settings for multiple fields. Manoj, I think createParser() and tokenizeSource() in DefauldCodeFormatter could use a review, because I wasn't sure what I'm doing. BTW, I had problems with updating my Eclipse. In bug 509257 comment 3 you've pointed me to the right binaries at the time, but I can't find a newer version. Is this described somewhere on the wiki?
(In reply to Mateusz Matela from comment #13) > BTW, I had problems with updating my Eclipse. In bug 509257 comment 3 you've > pointed me to the right binaries at the time, but I can't find a newer > version. Is this described somewhere on the wiki? Mateusz, the Y builds are meant for our internal consumption and not published. But you still can find the latest build information on jdt-core-dev mail archives. For convenience, the latest build is here: http://download.eclipse.org/eclipse/downloads/drops4/Y20170504-1000/
(In reply to Jay Arthanareeswaran from comment #14) > (In reply to Mateusz Matela from comment #13) > > BTW, I had problems with updating my Eclipse. In bug 509257 comment 3 you've > > pointed me to the right binaries at the time, but I can't find a newer > > version. Is this described somewhere on the wiki? > > Mateusz, the Y builds are meant for our internal consumption and not > published. @Jay: We have been publishing YBuilds since mid March. @Mateusz: You can download the latest Y build from: http://download.eclipse.org/eclipse/downloads/YIndex.php
(In reply to Mateusz Matela from comment #13) > Fixed with > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=49e9d0cec84062b8f937bcce9ab15ec576e3a013 > > > I've added CodeFormatter.K_MODULE_INFO constant as a new kind for mule info > files. It seemed more natural than adding a new property or method. Is it really necessary to add a new kind CodeFormatter.K_MODULE_INFO for this? As you mentioned in bug 516195, this will unnecessarily need a check to identify a module-info file at the places where formatter is called. Also, I don't see any special kind for the package-info files as well.
(In reply to Noopur Gupta from comment #16) Well, one way or another the formatter needs to know whether it's dealing with a modul-info, because it has to pass this information to a scanner and a parser that are created for analyzing the code (the information is currently passed by creating a fake compilation unit named module-info.java). We discussed this in comment 10 and 11. If you see some other approach, I'm open to further discussion. IMO if the parser won't be able to guess whether it's dealing with a module-info, it would be nice to have a new kind for module-info added in the parser's API. It would look much more natural than depending on the compilation unit's name, and currently choosing any other kind than COMPILATION_UNIT doesn't make any sense when parsing module-info. But that wouldn't solve the original issue, just a suggestion to Jay and Manoj.
> (In reply to Mateusz Matela from comment #17) >> (In reply to Noopur Gupta from comment #16) Manoj, could you please have a look and conclude on this?
(In reply to Noopur Gupta from comment #18) > > (In reply to Mateusz Matela from comment #17) > >> (In reply to Noopur Gupta from comment #16) > > Manoj, could you please have a look and conclude on this? Reopening. See bug 516195 comment #3 also.
(In reply to Noopur Gupta from comment #16) > (In reply to Mateusz Matela from comment #13) > > > > I've added CodeFormatter.K_MODULE_INFO constant as a new kind for mule info > > files. It seemed more natural than adding a new property or method. > > Is it really necessary to add a new kind CodeFormatter.K_MODULE_INFO for > this? As you mentioned in bug 516195, this will unnecessarily need a check > to identify a module-info file at the places where formatter is called. > Also, I don't see any special kind for the package-info files as well. @Noopur: The comparison may not be fair between a package-info.java file and a module-info.java file from the formatter's perspective. I don't see any special options for the package-info.java in formatter (@Mateusz - correct me if I am wrong in above) So formatter does not require to know whether this is a packege-info.java file or not - while in the case of module-info.java, module info specific formatting options exist and hence the formatter would need to know whether it is a module info file or not.
(In reply to Manoj Palat from comment #20) No no, the formatter is not the root of the problem here :) The problem is that (unless something has changed recently) the ASTParser (and Scanner) needs to be explicitly told whether it's dealing with a module-info or not. If the parser could figure it out by itself, the formatter would simply know what's going on from the contents of the parsed CompilationUnit (it would contain either a ModuleDeclaration or type declarations). I came up with the new kind CodeFormatter.K_MODULE_INFO because these kinds are basically used to determine how the formatter is supposed to invoke the ASTParser, so when there's a new way to invoke it for module-info, it seems the most natural to add a new kind. But that's just a tiny detail, if there was a boolean property in API or anything else, it would be equally problematic for jdt.ui. So unless you guys are planning some big changes in the parser, it seems the only viable solution is option 2 from comment 10. But still, in my uninformed opinion, if this kind of guessing gets implemented, it should be done as low as possible - somewhere in the ASTParser, not in the formatter.
Manoj, please enable Mateusz, so that he can continue on this next week. As mentioned before, a new kind is not OK.
(In reply to Mateusz Matela from comment #21) > (In reply to Manoj Palat from comment #20) > > No no, the formatter is not the root of the problem here :) True. I agree that formatter is just victim of a more rooted problem :) > The problem is that (unless something has changed recently) the ASTParser > (and Scanner) needs to be explicitly told whether it's dealing with a > module-info or not. If the parser could figure it out by itself, the > formatter would simply know what's going on from the contents of the parsed > CompilationUnit (it would contain either a ModuleDeclaration or type > declarations). This issue exists - Parser needs to be informed explicitly that this is indeed parsing a module-info. [It can't figure it out by itself] > > I came up with the new kind CodeFormatter.K_MODULE_INFO because these kinds > are basically used to determine how the formatter is supposed to invoke the > ASTParser, so when there's a new way to invoke it for module-info, it seems > the most natural to add a new kind. But that's just a tiny detail, if there > was a boolean property in API or anything else, it would be equally > problematic for jdt.ui. Given Dani's input on the new kind, the best option given the constraints is to abstract this in the ASTParser using a boolean property in API instead which woul again explicitly set/reset the module property. > So unless you guys are planning some big changes in the parser, it seems the > only viable solution is option 2 from comment 10. But still, in my > uninformed opinion, if this kind of guessing gets implemented, it should be > done as low as possible - somewhere in the ASTParser, not in the formatter. This may not be that straightforward and at best it could be a guesswork expecially if recovery is involved. (In reply to Dani Megert from comment #22) > Manoj, please enable Mateusz, so that he can continue on this next week. As > mentioned before, a new kind is not OK. @Dani: As mentioned above and as discussed offline, the boolean set API for ASTParser would be the option and we can remove the new kind. So I will provide an API from ASTParser to set the module info flag and would do this module-info simulation under the hood.
Manoj/Mateusz, please keep the concerns from bug 516195 into consideration while providing any API.
(In reply to Dani Megert from comment #22) > As mentioned before, a new kind is not OK. It's not OK because the entity invoking the formatter doesn't always know the compilation unit's name, right? It's not a simple matter of changing this new kind to some other flag in API. (In reply to Manoj Palat from comment #23) I still don't see a solution to the main problem. Adding a module-info flag in the ASTParser's API will only mean that the formatter won't need to create a fake compilation unit instance to parse a module-info. But it still doesn't know how to set this flag - it must either get this information from its invoking entity (a new kind or a new flag in the formatter's API, no difference), guess it by itself, or expect the parser to make the guess (this one seems to be impossible). Which way are we going? These are still the seme three options from comment 10.
(In reply to Mateusz Matela from comment #25) > (In reply to Dani Megert from comment #22) > > As mentioned before, a new kind is not OK. > > It's not OK because the entity invoking the formatter doesn't always know > the compilation unit's name, right? It's not a simple matter of changing > this new kind to some other flag in API. Since the new kind serves only the purpose of a boolean, the proposal to remove the new kind and replace it with a boolean. The formatter is expected to just pass that boolean instead of the new kind. > (In reply to Manoj Palat from comment #23) > > I still don't see a solution to the main problem. > > Adding a module-info flag in the ASTParser's API will only mean that the > formatter won't need to create a fake compilation unit instance to parse a > module-info. This proposal only pushes this task to the AST and nothing more/less. > But it still doesn't know how to set this flag - it must either get this > information from its invoking entity (a new kind or a new flag in the > formatter's API, no difference), At this point, yes. > guess it by itself, No, let the caller do this. [ need to address the concerns in bug 516195 comment 3 though] > or expect the parser to make the guess (this one seems to be impossible). At the current state of parsing, this solution is not possible. (This is option 3 listed in comment 10) > Which way are we going? These are still the seme three options from comment > 10. About the option 1, there is not much difference between this proposal and that since the decision is based on filename, and the result of that is consumed by the formatter in this proposal. And option 2 again (depends on parser) cannot be relied as we cannot be sure whether we will get to know whether this is a module-info.java or not.[As pointed out in comment 23, It would get trickier if recover is involved (ie if there are errors in the code and the parser needs to recover) At this point, if we do the following, we should get a solution: 1) remove the kind and replace it with a boolean in a new API 2) Push the code to ASTParser to use this boolean 3) Address the jdt.ui concerns in bug 516195 comment 3 - This is the current blocker. @Mateusz: Any other issues which you find?
(In reply to Manoj Palat from comment #26) > Since the new kind serves only the purpose of a boolean, the proposal to > remove the new kind and replace it with a boolean. That's not very convincing. It's not a fully independent boolean, because it could only be used with K_COMPILATION_UNIT and not other kinds. And it really feels like parsing module-info is a different mode (or "kind") of parsing - there's a separate syntax and different keywords. I don't want to defend my idea too strongly, it's your call in the end, just expressing my doubts. Also, we already have F_INCLUDE_COMMENTS, which serves only as a boolean flag, but is added to kind. Do you think we should do the same with module-info? As a result we would simply have F_MODULE_INFO instead of K_MODULE_INFO and declare it can only be used with K_COMPILATION_UNIT. > This proposal only pushes this task to the AST and nothing more/less. OK, it's still useful. Is there a bug for this change? > 1) remove the kind and replace it with a boolean in a new API > 2) Push the code to ASTParser to use this boolean > 3) Address the jdt.ui concerns in bug 516195 comment 3 - This is the current > blocker. OK, thanks for clearing that up. I thought the only reason for reopening this bug was to avoid changes in the formatter API altogether due to 516195 comment 3, but I see it's being addressed some other way.
Created attachment 269651 [details] Alternate solution An alternate solution now with the availability of filename is to provide this info in the JavaCore options - so the idea is: - jdt.core defines a new JavaCore option (COMPILER_SOURCE_PATH_ - jdt.ui formatter catches the filename and adds in the options (I will attach a jdt.ui WIP patch in bug 516195 immediately to try out) - DefCFOptions reads this option and sets the pathName field - DCF uses this option instead of checking for K_MODULE_INFO_KIND (using the same code written by Mateusz - at this point I am not talking of pushing that code to ast) The patch is wip and can be used to try out. Can discuss it out for a day to figure out issues if any for this solution @Mateusz : The conversion as a module takes place, but there is a AIOOBE Caused by: java.lang.ArrayIndexOutOfBoundsException: -1 at java.util.ArrayList.elementData(ArrayList.java:418) at java.util.ArrayList.get(ArrayList.java:431) at org.eclipse.jdt.internal.formatter.TokenManager.get(TokenManager.java:73) at org.eclipse.jdt.internal.formatter.TokenManager.findIndex(TokenManager.java:166) at org.eclipse.jdt.internal.formatter.TokenManager.firstIndexBefore(TokenManager.java:221) at org.eclipse.jdt.internal.formatter.linewrap.WrapPreparator.handleModuleStatement(WrapPreparator.java:809) at org.eclipse.jdt.internal.formatter.linewrap.WrapPreparator.visit(WrapPreparator.java:802) at org.eclipse.jdt.core.dom.ProvidesDirective.accept0(ProvidesDirective.java:156) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2800) at org.eclipse.jdt.core.dom.ASTNode.acceptChildren(ASTNode.java:2871) at org.eclipse.jdt.core.dom.ModuleDeclaration.accept0(ModuleDeclaration.java:226) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2800) at org.eclipse.jdt.core.dom.ASTNode.acceptChild(ASTNode.java:2848) at org.eclipse.jdt.core.dom.CompilationUnit.accept0(CompilationUnit.java:257) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2800) at org.eclipse.jdt.internal.formatter.DefaultCodeFormatter.prepareWraps(DefaultCodeFormatter.java:405) at org.eclipse.jdt.internal.formatter.DefaultCodeFormatter.prepareFormattedCode(DefaultCodeFormatter.java:223) at org.eclipse.jdt.internal.formatter.DefaultCodeFormatter.format(DefaultCodeFormatter.java:177) at org.eclipse.jdt.internal.formatter.DefaultCodeFormatter.format(DefaultCodeFormatter.java:160) at org.eclipse.jdt.internal.corext.util.CodeFormatterUtil.reformat(CodeFormatterUtil.java:288) at org.eclipse.jdt.internal.ui.text.java.JavaFormattingStrategy.format(JavaFormattingStrategy.java:67) [you will require the jdt.ui also to be patched]
New Gerrit change created: https://git.eclipse.org/r/102434
(In reply to Manoj Palat from comment #28) > Created attachment 269651 [details] [diff] > Alternate solution That's a really bad solution for several reasons. - From the name of the constant 'COMPILER_SOURCE_PATH' no one will ever have a clue what to do with this. At least too complex for my little brain ;-) - It would belong to a formatter class/interface not JavaCore. - Giving the path seems wrong. To me it looks like the simplest fix is to just add CodeFormatter#formatModuleInfoFile.
(In reply to Manoj Palat from comment #28) > @Mateusz : The conversion as a module takes place, but there is a AIOOBE That's because DefaultCodeFormatter.tokenizeSource() also checks for module-info so it needs to be modified too.
(In reply to Dani Megert from comment #30) > (In reply to Manoj Palat from comment #28) > > Created attachment 269651 [details] [diff] [diff] > > Alternate solution > Noting that I've moved the assignee back to Mateusz since he put in the bulk of the changes and my (small snippet of) commit would just address to enable the blocker (if the alternate solution gets committed) and further, would require Mateusz's help to complete the solution. [Have put myself as QA instead though this is really not required with Mateusz being the author [and the expert] of the formatter] > That's a really bad solution for several reasons. Thanks Dani for the feedback. However, please reply to the comments inlined. > - From the name of the constant 'COMPILER_SOURCE_PATH' no one will ever have > a clue what to do with this. At least too complex for my little brain ;-) Please suggest a name that is more appropriate > - It would belong to a formatter class/interface not JavaCore. This information has nothing to do with the formatter - It gets used up by the compiler implicitly - primarily because of the limitation of parser which is a part of the compiler. And hence logically this is a message to the compiler and hence I chose the name COMPILER_SOURCE_PATH - can change this to a more appropriate name if it is suggested from the previous comment. > - Giving the path seems wrong. Why is it wrong? Could you please explain? Please note that I am not giving the PATH or IPATH here, but only the path as a String, if that was the concern. [An alternate is to pass just the filename if there are any issues in sending the whole pathname] > To me it looks like the simplest fix is to just add > CodeFormatter#formatModuleInfoFile. How is this different in principle from passing a new KIND? (Note that the alternate solution did away with a new KIND and would use the K_COMPILATION_.. kind itself). Moreover, I see the following advantages with the pathname instead of this as listed below: (a) With the whole pathname passed, I don't see an issue if at a future point in time the module-info.java filename is changed to something else (b) Assuming that you meant a boolean there, the decision whether something is a module or not would be done at jdt.ui, while with the whole pathname being passed, that decision will be done at the compiler/parser side with no additional code where would be the "more" rightful place to take that decision.
(In reply to Manoj Palat from comment #32) > (In reply to Dani Megert from comment #30) > > - Giving the path seems wrong. > Why is it wrong? Could you please explain? For me it also feels wrong - the options map should contain global java compiler and formatter settings, not something that changes from file to file. For example, if I wanted to run a few formatters in parallel, I'd need to be careful to have a separate options map for each formatter - that's counterintuitive. The confusion it could cause doesn't seem worth the benefits you listed. > > To me it looks like the simplest fix is to just add > > CodeFormatter#formatModuleInfoFile. > > How is this different in principle from passing a new KIND? I'm also interested in why the new kind is so bad. For me it feels equally intuitive and a bit more convenient for caller than a separate method.
Manoj, I explained to you last week why the kind is not working and you agreed. Please refresh your memory.
(In reply to Dani Megert from comment #34) > Manoj, I explained to you last week why the kind is not working and you > agreed. Please refresh your memory. Now you make it sound like it's some kind of a secret you're embarrassed to talk about in public ;) After discussing a bug outside bugzilla, it's nice to post potentially non-obvious conclusions in a comment here - keeps the others in the loop and helps when anyone forgets... I came upon one advantage of the new kind versus a separate method - it preserves the usefulness of the K_UNKNOWN kind. The point of this kind is that the caller doesn't need to know what's in the source code being formatted. With a separate method, the caller will always need to know whether it's handling module-infos.
Gerrit change https://git.eclipse.org/r/102434 was merged to [BETA_JAVA9]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7cd689e47acb28ea4d32dafcdb91c140b9faaa25
(In reply to Eclipse Genie from comment #36) > Gerrit change https://git.eclipse.org/r/102434 was merged to [BETA_JAVA9]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=7cd689e47acb28ea4d32dafcdb91c140b9faaa25 Manoj, you need to check your local compiler preferences. This introduced a compiler error (albeit in Javadoc) about missing @since tags. Also there are couple of compatibility problems, which I am not sure why.
(In reply to Eclipse Genie from comment #36) > Gerrit change https://git.eclipse.org/r/102434 was merged to [BETA_JAVA9]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7cd689e47acb28ea4d32dafcdb91c140b9faaa25 > Looking from the side lines this: this.isInModuleInfo = true; without ever resetting the flag looks suspicious.
New Gerrit change created: https://git.eclipse.org/r/102644
reverted the commit to address 37 and 38
(In reply to Jay Arthanareeswaran from comment #37) > (In reply to Eclipse Genie from comment #36) > > Gerrit change https://git.eclipse.org/r/102434 was merged to [BETA_JAVA9]. > > Commit: > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > > ?id=7cd689e47acb28ea4d32dafcdb91c140b9faaa25 > > Manoj, you need to check your local compiler preferences. This introduced a > compiler error (albeit in Javadoc) about missing @since tags. Also there are > couple of compatibility problems, which I am not sure why. @Jay: If you are referring to this API filter issue, then this is expected until the new kind is removed: "Description Resource Path Location Type The API problem filter for: 'The field org.eclipse.jdt.core.formatter.CodeFormatter.K_MODULE_INFO has been added to a class' is no longer used CodeFormatter.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/core/formatter line 29 Unused API Problem Filter Problem ". This patch just provides an API as per the suggestion in comment 30. Further work needs to be done once. I didn't find the compiler error in my local setting - Could you please paste the errors you see ?
(In reply to Manoj Palat from comment #41) > (In reply to Jay Arthanareeswaran from comment #37) > > (In reply to Eclipse Genie from comment #36) > > > Gerrit change https://git.eclipse.org/r/102434 was merged to [BETA_JAVA9]. > > > Commit: > > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > > > ?id=7cd689e47acb28ea4d32dafcdb91c140b9faaa25 > > > > Manoj, you need to check your local compiler preferences. This introduced a > > compiler error (albeit in Javadoc) about missing @since tags. Also there are > > couple of compatibility problems, which I am not sure why. > > @Jay: If you are referring to this API filter issue, then this is expected > until the new kind is removed: > > "Description Resource Path Location Type > The API problem filter for: 'The field > org.eclipse.jdt.core.formatter.CodeFormatter.K_MODULE_INFO has been added to > a class' is no longer used CodeFormatter.java > /org.eclipse.jdt.core/formatter/org/eclipse/jdt/core/formatter line 29 > Unused API Problem Filter Problem > ". > This patch just provides an API as per the suggestion in comment 30. Further > work needs to be done once. I didn't find the compiler error in my local > setting - Could you please paste the errors you see ? hmm.. I see these now... my (bad)baseline was not oxygen when I tried this.. : Description Resource Path Location Type Missing @since tag on formatModuleInfoFile(int, String, int, int, int, String) CodeFormatter.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/core/formatter line 326 @since tag problem The method org.eclipse.jdt.core.formatter.CodeFormatter.formatModuleInfoFile(int, String, int, int, int, String) that has to be implemented has been added CodeFormatter.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/core/formatter line 326 Compatibility Problem The method org.eclipse.jdt.core.formatter.CodeFormatter.formatModuleInfoFile(int, String, IRegion[], int, String) that has to be implemented has been added CodeFormatter.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/core/formatter line 364 Compatibility Problem The major version should be incremented in version 3.13.0, since API breakage occurred since version 3.13.0 MANIFEST.MF /org.eclipse.jdt.core/META-INF line 6 Version Numbering Problem I believe these are what you are talking about.
New Gerrit change created: https://git.eclipse.org/r/102646
(In reply to Mateusz Matela from comment #33) > (In reply to Manoj Palat from comment #32) > > (In reply to Dani Megert from comment #30) > > > To me it looks like the simplest fix is to just add > > > CodeFormatter#formatModuleInfoFile. > > > > I'm also interested in why the new kind is so bad. For me it feels equally > intuitive and a bit more convenient for caller than a separate method. @Mateusz: Had a chance to talk to Dani; updating the details of the discussion here: Dani's primary premise against a new kind was that a kind is always concerned about a normal compilation unit (not module info) and if we introduced a new kind then we would have to document where each of the kinds will be applicable (currently all the kinds are applicable in a CU) That was the reason he proposed the new api that applies only to module-info - CodeFormatter#formatModuleInfoFile(). And he meant this api not to have the kind information. However, we discussed the issue of F_INCLUDE_COMMENTS. With the case of comments being processed for formatting in the module-info, the new api would still need another parameter to pass this information - Given this case he is fine to have the new kind K_MODULE_INFO (and not to have the new api).
(In reply to Manoj Palat from comment #44) > @Mateusz: Had a chance to talk to Dani; updating the details of the > discussion here: Dani's primary premise against a new kind was that a kind > is always concerned about a normal compilation unit (not module info) and if > we introduced a new kind then we would have to document where each of the > kinds will be applicable (currently all the kinds are applicable in a CU) > That was the reason he proposed the new api that applies only to module-info > - CodeFormatter#formatModuleInfoFile(). And he meant this api not to have > the kind information. However, we discussed the issue of F_INCLUDE_COMMENTS. > With the case of comments being processed for formatting in the module-info, > the new api would still need another parameter to pass this information - > Given this case he is fine to have the new kind K_MODULE_INFO (and not to > have the new api). Did you verify that F_INCLUDE_COMMENTS and other kinds are honored? In a previous discussion it was said that this isn't the case, which spoke against using any kind to format module-info. If it *is* implemented, then adding a new kind for module-info is fine, but we must document which kind is applicable to which file type. Otherwise a new method that does not take a kind parameter is the right fix.
(In reply to Dani Megert from comment #45) > (In reply to Manoj Palat from comment #44) > > @Mateusz: Had a chance to talk to Dani; updating the details of the > > discussion here: Dani's primary premise against a new kind was that a kind > > is always concerned about a normal compilation unit (not module info) and if > > we introduced a new kind then we would have to document where each of the > > kinds will be applicable (currently all the kinds are applicable in a CU) > > That was the reason he proposed the new api that applies only to module-info > > - CodeFormatter#formatModuleInfoFile(). And he meant this api not to have > > the kind information. However, we discussed the issue of F_INCLUDE_COMMENTS. > > With the case of comments being processed for formatting in the module-info, > > the new api would still need another parameter to pass this information - > > Given this case he is fine to have the new kind K_MODULE_INFO (and not to > > have the new api). > > Did you verify that F_INCLUDE_COMMENTS and other kinds are honored? In a > previous discussion it was said that this isn't the case, which spoke > against using any kind to format module-info. If it *is* implemented, then > adding a new kind for module-info is fine, but we must document which kind > is applicable to which file type. Otherwise a new method that does not take > a kind parameter is the right fix. Yes. Verified that F_INCLUDE_COMMENTS is honored (comments get formatted) when used with the new kind. Mentioned in the javadoc of K_MODULE_INFO also. (Other kinds I believe you meant other individual comment kinds - we cannot pass these individual kinds to a module-info code since the parser cannot figure out its a module-info or not - probably this was the point discussed). However as mentioned above, the comments do get formatted using the CodeFormatter.K_MODULE_INFO | CodeFormatter.F_INCLUDE_COMMENTS. This is implemented already. Yes, we need to document the applicability of different kinds. @Mateusz: Could you please take this feature to completion? Will reopen the jdt.ui part as well?
(In reply to Manoj Palat from comment #46) > > Did you verify that F_INCLUDE_COMMENTS and other kinds are honored? In a > > previous discussion it was said that this isn't the case, which spoke > > against using any kind to format module-info. If it *is* implemented, then > > adding a new kind for module-info is fine, but we must document which kind > > is applicable to which file type. Otherwise a new method that does not take > > a kind parameter is the right fix. > > Yes. Verified that F_INCLUDE_COMMENTS is honored (comments get formatted) > when used with the new kind. Mentioned in the javadoc of K_MODULE_INFO also. > (Other kinds I believe you meant other individual comment kinds - we cannot > pass these individual kinds to a module-info code since the parser cannot > figure out its a module-info or not - probably this was the point > discussed). However as mentioned above, the comments do get formatted using > the CodeFormatter.K_MODULE_INFO | CodeFormatter.F_INCLUDE_COMMENTS. This is > implemented already. > Yes, we need to document the applicability of different kinds. > @Mateusz: Could you please take this feature to completion? Will reopen the > jdt.ui part as well? OK, with this, I'm fine adding the new kind.
(In reply to Manoj Palat from comment #46) > @Mateusz: Could you please take this feature to completion? Will reopen the > jdt.ui part as well? OK, I should have some time today and over the weekend to work on this. I've improved the javadoc a bit with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=4d69e622c685dab4bb529a73447c5cb9832cf2e5. Please let me know whether you find it sufficient. By the way, I'm sure glad my idea survived in the end, but still don't think the reasoning was valid. If I understood Manoj correctly (and he understood Dani correctly) the main point was a need for documentation of the new kind? But a new method would also have to be documented and I don't see how this would make things any simpler. Note that the new kind only affects K_COMPILATION_UNIT, because there are no body declarations, statements or expressions in module-infos (or at least the parser currently doesn't work with these kinds). If the only counterargument was that the new method needs the kind parameter to set F_INCLUDE_COMMENT flag, then we could just declare the new method always works as if this flag is set (and in rare cases when someone doesn't want comments formatted, they can just switch it off using standard formatter settings). And then the other point was that apparently some kinds are ignored anyway so we shouldn't develop in this direction any further? If that was the case, we should rather fix this and make the formatter work as documented, no?
(In reply to Mateusz Matela from comment #48) > (In reply to Manoj Palat from comment #46) > > @Mateusz: Could you please take this feature to completion? Will reopen the > > jdt.ui part as well? > > OK, I should have some time today and over the weekend to work on this. > I've improved the javadoc a bit with > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=4d69e622c685dab4bb529a73447c5cb9832cf2e5. > > Please let me know whether you find it sufficient. Thanks Mateusz! A few suggestions 1) line 224 and 267: <b>Since 3.4</b> for {@link #K_MODULE_INFO} - needs to be changed to 3.13 BETA_JAVA9 since K_MODULE_INFO is being introduced only now. 2)Additional documentation mentioning the applicability of each of the other kinds should be added (saying that these kinds applicable only in non module-info compilation units) > By the way, I'm sure glad my idea survived in the end, but still don't think > the reasoning was valid. If I understood Manoj correctly (and he understood > Dani correctly) the main point was a need for documentation of the new kind? > But a new method would also have to be documented and I don't see how this > would make things any simpler. Note that the new kind only affects > K_COMPILATION_UNIT, because there are no body declarations, statements or > expressions in module-infos (or at least the parser currently doesn't work > with these kinds). If the only counterargument was that the new method needs > the kind parameter to set F_INCLUDE_COMMENT flag, then we could just declare > the new method always works as if this flag is set (and in rare cases when > someone doesn't want comments formatted, they can just switch it off using > standard formatter settings). > > And then the other point was that apparently some kinds are ignored anyway > so we shouldn't develop in this direction any further? If that was the case, > we should rather fix this and make the formatter work as documented, no? At a later point, we might have new kinds akin to statements or expressions for module-info, if we find a need. Though this point can be countered as well with an alternative method, I would suggest that given that we have an agreement currently on the API and given the time constraints of java 9 release, let us stick to the agreed upon decision. This would also help us to iron out issues if any in the implementation before the final release. And thanks again Mateusz for taking this up!
(In reply to Manoj Palat from comment #49) > Thanks Mateusz! A few suggestions > 1) line 224 and 267: <b>Since 3.4</b> for {@link #K_MODULE_INFO} - needs to > be changed to 3.13 BETA_JAVA9 since K_MODULE_INFO is being introduced only > now. Oh, it was actually supposed to say #K_COMPILATION_UNIT instead of #_KMODULE_INFO > 2)Additional documentation mentioning the applicability of each of the other > kinds should be added (saying that these kinds applicable only in non > module-info compilation units) OK, I'll add it in K_EXPRESSION, K_STATEMENTS and K_CLASS_BODY_DECLARATION. For comment kinds it's too obvious IMO (if something is a comment, it cannot be a module-info). > At a later point, we might have new kinds akin to statements or expressions > for module-info, if we find a need. This makes me think we should maybe replace the K_MODULE_INFO kind with F_MODULE_INFO flag, as suggested in comment 27. Otherwise, if we add support for statements or expressions, we will have kinds like K_MODULE_INFO_STATEMENTS, which will look odd. With a flag, we can just declare it can be used with additional kinds. What do you think? I guess it depends on probability that we will actually add these features.
(In reply to Mateusz Matela from comment #50) > (In reply to Manoj Palat from comment #49) > This makes me think we should maybe replace the K_MODULE_INFO kind with > F_MODULE_INFO flag, as suggested in comment 27. Otherwise, if we add support > for statements or expressions, we will have kinds like > K_MODULE_INFO_STATEMENTS, which will look odd. With a flag, we can just > declare it can be used with additional kinds. What do you think? I guess it > depends on probability that we will actually add these features. As mentioned in previous comment, let us stick to the agreed upon solution. Let K_MODULE_INFO be there similar to K_COMPILATION_UNIT.
Fixed the javadoc with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=6d88b8b3b3489dcbd48fd8b5528ae20c4ee31f39, also removed an old API error filter. With http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=a12a8b06ed46f08c4f2334d2bb7677bd8e3f0433 I fixed two things spotted while analyzing formatter call hierarchy for bug 516195.
