Package: emacs;
Reported by: Artem <snake05865 <at> gmail.com>
Date: Sat, 28 Dec 2024 04:38:02 UTC
Severity: minor
Found in version 31.0.50
View this message in rfc822 format
From: Eli Zaretskii <eliz <at> gnu.org> To: Yuan Fu <casouri <at> gmail.com> Cc: 75154 <at> debbugs.gnu.org, theo <at> thornhill.no, stefankangas <at> gmail.com, snake05865 <at> gmail.com Subject: bug#75154: 31.0.50; java-ts-mode. Issues with Indentation Date: Sat, 01 Mar 2025 14:06:13 +0200
Ping! Can we make some further progress here?
> From: Yuan Fu <casouri <at> gmail.com>
> Date: Thu, 13 Feb 2025 22:24:06 -0800
> Cc: Theodor Thornhill <theo <at> thornhill.no>,
> 75154 <at> debbugs.gnu.org,
> Eli Zaretskii <eliz <at> gnu.org>,
> Stefan Kangas <stefankangas <at> gmail.com>
>
> > On Jan 28, 2025, at 6:06 AM, Artem <snake05865 <at> gmail.com> wrote:
> >
> > Okay, you may personally disagree. But why not create a separate
> > setting tailored to your preferences? You seem to agree that IntelliJ IDEA
> > is currently the canonical editor for Java. Set 8 spaces as the default and
> > add an option to adjust it.
> >
> > Do I need to provide examples of major open-source projects that use 8 spaces?
> > For instance, Apache Tomcat:
> > https://github.com/apache/tomcat/blob/cd997770105ea960764c3d7844e13c0bbe8fd3c1/java/org/apache/juli/OneLineFormatter.java#L102
> > Or Red Hat Quarkus:
> > https://github.com/quarkusio/quarkus/blob/0e013fabfc14557e688b559f6edea43b8934406a/devtools/cli/src/main/java/io/quarkus/cli/CliPlugins.java#L37
> >
> > I think this is a very important matter. Personally,
> > it drives me crazy when the spacing is off.
> >
> >> While this is probably catchable, the "workaround" I started using is to
> >> end the statement with a semi, then RET:
> >>
> >> ```
> >> class Foo {
> >> void Foo() {
> >> List<Customer> customers = customer
> >> |; // Point is now at |
> >>
> >> }
> >> }
> >> ```
> >>
> >> Now we have a valid parse tree, indented correctly, and can continue
> >> chaining. I agree that this very much is a hack.
> >
> > Yes, it does look unnatural. And you know, there’s a similar issue in bash-ts-mode.
> > I can’t show the exact code snippet right now, but the behavior is essentially the same.
> > You press RET, the cursor ends up in the wrong position, then press RET again,
> > and the code block jumps to where it’s supposed to be.
> > Where’s the error? Is it a problem with the mode’s implementation
> > or a bug in Tree-sitter itself? Or did everyone just borrow logic from
> > each other, and now this indentation behavior has spread to other modes?
> >
> >>
> >>> Moreover, the current indentation is hardcoded and doesn’t allow
> >>> flexibility. For instance, in python-ts-mode, pressing TAB allows you to
> >>> adjust constructions more freely.
> >>> This level of flexibility would be beneficial in this context
> >>>
> >>> This inflexibility prevents writing common patterns like:
> >>> @Override
> >>> protected void configure(HttpSecurity http) throws Exception {
> >>> http
> >>> .authorizeRequests()
> >>> .antMatchers("/admin/**").hasAuthority("ADMIN")
> >>> .antMatchers("/user").hasAnyAuthority("USER", "ADMIN")
> >>> .antMatchers("/", "/index").permitAll()
> >>> Example 2:
> >>>
> >>> The following looks correct -
> >>>
> >>> public class FloodFill {
> >>> public static void main(String[] args) {
> >>> List<Foo> stream = students.stream()
> >>> .filter(item -> {
> >>> return item.getValue() > 100 &&
> >>> item.isActive();
> >>> })
> >>> .map()
> >>> .collect();
> >>> }
> >>> }
> >>>
> >>> But java-ts-mode produces:
> >>>
> >>> public class FloodFill {
> >>> public static void main(String[] args) {
> >>> List<Foo> stream = students.stream()
> >>> .filter(item -> {
> >>> return item.getValue() > 100 &&
> >>> item.isActive();
> >>> })
> >>
> >> Yes, this really annoys me too, so I should try to fix this. I don't
> >> remember from the top of my head what the problem was, but it forced me
> >> either to omit the curlies, or just extract the body into a function and
> >> pass it. But we should absolutely fix this. I believe there were several
> >> difficulties here, where some are related to incomplete statements, and
> >> some is related to treesit.el and the java parser idiosyncrasies. But
> >> this is a valid bug in an of itself. If not too much of a hassle, could
> >> you create a separate bugreport just for this case?
> >>
> >>> .map()
> >>> .collect();
> >>> }
> >>> }
> >
> > I’m not sure. Maybe I can create a new bug report? But honestly,
> > I don’t understand the point. A new report just to track the resolution
> > of some minor issue that annoys you and that you consider more important than others?
> >
> >>>
> >>> 2) Inner Classes
> >>> Example 1:
> >>> public class Outer {
> >>> class Inner {| <-- cursor here moves Inner class unexpectedly
> >>>
> >>> }
> >>> }
> >>> Example 2:
> >>> public class Outer {
> >>> class Inner { // ???
> >>> | <-- cursor here.
> >>> }
> >>> }
> >>>
> >>> Why does this happen? I did not request this behavior. While Example 1
> >>> demonstrates bad code style, it is technically valid. Such "magical"
> >>> formatting should be handled by a Java formatter, not by Emacs or
> >>> Tree-sitter rules.
> >>> IntelliJ IDEA does not apply such formatting; it leaves this task to the formatter.
> >>>
> >>
> >> This is due to electric indent. You could disable it yourself in your
> >> config or just live with it.
> >
> > Yes, I’ve already figured that out, thank you. But what does it give me? Am I supposed
> > to just live with it? By the way, I tried disabling electric-indent-mode, but then indentation
> > stopped working entirely. I briefly went through the documentation to understand what’s going on,
> > and from what I gathered, this mode uses various backends, including Tree-sitter now.
> >
> > I could be wrong, but it seems like electric-indent-mode needs a review and a rethinking
> > of how it should work, considering Tree-sitter is being actively used.
> >
> > And the extra “magic” performed by electric-indent-mode might have been relevant a long
> > time ago, or maybe it still is in some cases. But for Java/C/C++, I don’t see why
> > it’s necessary. It’s just annoying and gets in the way.
> >
> >>> Example 3:
> >>> public class Outer {
> >>> class Inner{
> >>> void foo(){
> >>> }|<--start position. RET
> >>> |<-- expected position
> >>> |<-- actual
> >>> }
> >>> }
> >>>
> >>> If Inner class has incorrect indentation, subsequent code will also be incorrectly indented.
> >>>
> >>
> >> I would rather inverse the statement, and say that subsequent code is
> >> correctly indented, but the preceding code is ignored. As the correct
> >> code is
> >> ```
> >> public class Outer {
> >> class Inner{
> >> void foo(){
> >> }
> >>
> >> }
> >> }
> >> ```
> >>
> >> Indentation at column 8 is expected here, IMO. I'm not sure we should
> >> try to work around clearly "incorrectly" indented code.
> >
> > In my understanding, and in IntelliJ IDEA’s, it should work so that
> > it doesn’t matter what syntactic construct, method, or anything else is far
> > behind—several levels above, to be exact. Tree-sitter should only care
> > about what’s one step behind you. It shouldn’t matter to you that the Inner
> > class is misplaced. Your method foo() should focus on where it belongs, even
> > if the Inner class has incorrect indentation. This isn’t a hack. It feels
> > natural—just follow what’s next to you.
> > Right now, it’s just a tightly coupled construct where breaking one
> > thing breaks another—or everything altogether.
> >
> >>
> >>>
> >>> 4) Java 15 text blocks
> >>> Text blocks are not properly handled
> >>>
> >>> public class TextBlocks {
> >>> System.out.println(ctx.fetch("""
> >>> SELECT table_schema, count(*)
> >>> FROM information_schema.tables
> >>> """));
> >>> }
> >>
> >> This isn't valid java is it? I changed it to
> >>
> >
> > Yes, this is invalid code. Again, this touches on how indentation works.
> > The machine and the code should think like me:
> > "You see I’ve placed triple quotes? You see there’s a method earlier?
> > Guess what I’m about to write? Of course, it’s going to be a TEXT BLOCK! Bingo!"
> > There’s nothing else I could possibly write there.
> >
> >> ```
> >> public class TextBlocks {
> >> public void foo() {
> >> System.out.println(ctx.fetch("""
> >> SELECT table_schema, count(*)
> >> FROM information_schema.tables
> >> """));
> >>
> >> }
> >> }
> >> ```
> >
> > Even if I write this code correctly, I still don't have the proper indentation.
> > Did you configure anything additional on your end? Out of the box, this doesn't work for me
> >
> > public class TextBlocks {
> > void foo() {
> > System.out.println(ctx.fetch(""" <-- 1. press RET
> > | <-- and you will be here (Actual)
> > | <-- Expected
> > """));
> > }
> > }
> >
> > java-mode, unlike java-ts-mode, is a bit smarter in this regard - the indentation is at least
> > somewhat recognized, but it still ends up in the wrong place. Manual adjustment of rules is required.
> >
> >> I usually indent stuff like this marking the region then using C-x i
> >> then <left> or <right>
> > What is C-x i? For me it's insert-file.
> >>
> >>> - New SQL expressions should be sticky
> >> What do you mean here? Align the newlines to the previous line?
> >
> > I've already forgotten what I meant. But yes, sticky means aligning the new line according
> > to the previous line. I think I saw this somewhere or maybe
> > I'm confusing it with Stream API method chains.
> >
> >>> It seems such multiline strings also do not work well, for example:
> >>> "'The time has come,' the Walrus said,\n" +
> >>
> >> Not sure I understand what you mean here
> >>
> >
> > Looks like I said too much here. I apologize. Yes, this works correctly.
> > For example, such code will make proper indentation
> > String sql = "SELECT u.id, u.name, u.email, COUNT(o.id) AS order_count " +
> > "FROM users u " +
> > "LEFT JOIN orders o ON u.id = o.user_id " +
> >
> >>> 6) Multiple Parameters in Methods
> >>> Example 1
> >>> public record StudentRecord(
> >>> String firstName,
> >>> String lastName,
> >>> Long studentId,
> >>> String email)
> >>
> >> What is wrong here?
> >
> > There's nothing broken in the code above. But in java-ts-mode it looks like this:
> >
> > public record StudentRecord(
> > String a,
> > String b, <--Actual (BAD)
> > String b <--Expected (Good)
> > )
> >
> >>> Example 2
> >>> public String filterData(@RequestParam(required = false) String name,
> >>> @RequestParam(required = false) String name,
> >>> @RequestParam(required = false) Integer age
> >>> )
> >>> java-ts-mode fails to handle these cases correctly.
> >>>
> >>
> >> How so?
> > Current behavior in java-ts-mode:
> > public String filterData(@RequestParam(required = false) String name,
> > @RequestParam(required = false) String name, <--Actual
> > @RequestParam(required = false) <--Expected
> > )
> >
> > You understand, I can't even press TAB to move this over?
> >
> >>> Desired Fontification (Out of the Box):
> >>> - Annotations (@Annotations)
> >>> - Diamond Brackets (<>)
> >>> - Constants, Static Variables, Enum Variables should be highlighted with
> >>> distinct colors and optionally italic font
> >>> - Unused Variables or Classes (Grayed Out)
> >>
> >> This feels more like opinions rather than errors, but feature requests
> >> are always welcome, of course. You could try to add patches for some of
> >> these that I can review?
> >
> > These might be my personal wishes. But didn't some of this work out of the box in java-mode?
> > Am I right about this? Why did this disappear in java-ts-mode?
> > Actually, I wouldn't worry about this if I could simply press M-x customize and select
> > the needed colors. But... For some reason it's not that simple. In general,
> > custom color highlighting setup isn't very oriented towards people without Elisp knowledge.
> > Doesn't Emacs philosophy assume that many settings should be done this way
> > and all of this is then saved in custom.el?
> >
> >>> - Unused variables, unused classes, etc., highlighted in gray. Not sure if this can be achieved
> >>> with Tree-sitter. Anyway, with Flymake + Eglot, it currently works in a somewhat clunky manner.
> >>>
> >>
> >> This isn't possible with tree sitter. How is eglot clunky here? I'm kind
> >> of satisfied, at least when the lsp actually marks these.
> >>
> >>> Example
> >>> public class TextBlocks {
> >>> enum AnEnum { CONST1, CONST2 } <-- No effect for unused AnEnum.
> >>> public static final String HELLO ="HELLO";
> >>>
> >>> public static void main(String[] args) {
> >>> int i = 0; <-- Flymake identifies as unused but looks unpolished.
> >>> System.out.println(HELLO);
> >>> }
> >>> }
> >>>
> >>
> >> This isn't treesit related, but could be a report for flymake
> >> maintainers. We need to get the information from the server, though.
> >
> > You know what I thought about? I thought that since treesitter
> > builds a syntax tree of the file, it should be able to know what
> > in this tree has no usage and isn't connected to anything.
> >
> >>> Overall:
> >>>
> >>> I may have missed some aspects, but as it stands, Emacs is not
> >>> comfortable for Java development with these issues.
> >>>
> >>
> >> Can you provide some sort of priority here? Most feel cosmetic, but
> >> there are some real bugs here. One issue I also want, but is not quite
> >> sure how to fix yet is the inline sql. We could try to do some multi
> >> mode sql syntax highlighting here, possibly.
> >
> > What priority for each problem? For me, everything is priority.
> > Maybe for someone who understands Elisp well, some things might be less urgent.
> > Very very very many people don't write anything in the bug tracker.
> > Because many things can really look like the user just needs to open
> > the Emacs Lisp references manual and do it themselves, rather than asking someone to do it for them.
> >
> > Slightly off-topic....
> >
> > Actually, I realized long ago that many things in Emacs are made so that users are given
> > some sandbox and provided opportunities to play in it themselves and customize it to their taste.
> > But there aren't many things that are really polished and designed to just open and use.
> > I haven't tried many programming modes. But recently I discovered Racket-mode.
> > God, how neat, polished, well-thought-out this mode looks and requires almost no additional setup
> > from the user, and tons of documentation is written.
> > For Java, which is in the top 3 languages worldwide, there is NOTHING in Emacs.
> > More precisely, there was something and it's probably gathering dust somewhere deep in Emacs' depths,
> > something like JDEE or CEDET which 1-2 people use. And nobody talks about this now and nowhere
> > is it written about. You need to do some research work to understand what was relevant before
> > and what wasn't.
> > Do you understand that there isn't even a "run main" button?
> > This is funny and sad.
> >
> >> As another data point - I use Emacs for java development as the sole
> >> emacs user on my team in a sea of Intellij users, and I don't really
> >> share the view that it isn't comfortable for Java development.
> >
> > That's cool. I want to be like you and be the only Java developer who works in Emacs,
> > but reality is quite different for now. There's so much! So much! That needs to be finished.
> > Well okay, I'm lying, not that much.
> > I think attaching transient.el + hydra.el + a couple dozen functions and this should
> > become almost indistinguishable from IDE and maybe even superior in some places.
> > But this still needs to be done...
> >
> >> There is
> >> a quirk here and there, sure, but I'm just as productive as everyone
> >> else there. For me the biggest issue is the one case you mentioned here
> >> with nested blocks inside of a chain of methods.
> >>
> >>
> >> I'm not sure we should consider what Intellij does as a factual
> >> source. Though it is for now the canonical java editor, who knows for
> >> how long, and it feels time consuming to jump after a moving target.
> >
> > As soon as there's some interested person who wants to do some refactoring
> > of everything that was previously done for JAVA in Emacs
> > and rewrites/writes/adapts it to new realities.
> > I'll be able to officially declare that Emacs will be the best editor/IDE for Java.
> > But for now, that person hasn't been born yet. Or maybe it all exists already,
> > but somewhere in personal repositories on GitHub or somewhere else,
> > but won't make it into the core of java-ts-mode.
> >
> > I know you're not the only one who writes Java in Emacs and remains productive.
> > But... I expressed my thoughts about the situation earlier.
> > And you understand these are thoughts of an ordinary person, not an Emacs hacker.
> > If you can do it, it doesn't mean that thousands of others can do exactly the same.
>
>
> Ok, I attached a patch-set, if you apply this patch-set on top of _latest master_, most of the fixable problems in this report should be fixed/improved. Now, let me reply each item:
>
> 1. In the patch I added java-ts-mode-method-chaining-indent-offset,
> defaults to 8
>
> 2. Set electric-indent-chars to nil so it doesn’t indent the current
> line when you press RET.
>
> 3. The indentation is fixed once you type a valid statement, this is
> because tree-sitter needs something to generate a parse-tree. We
> can add some heuristics that gives a more intuitive indentation
> when the line is empty, eg, "if prev line is while and current line
> is empty, indent one level", etc. But that’s a more complicated
> feature and I’ll defer to Theo.
>
> 4.
> - Triple quote support in electric-pair-mode -> let’s open a separate
> bug report for it and have electric-pair-mode maintainer take a
> look.
>
> - In general, TAB in Emacs prog modes indent to a fixed point, rather
> than just inserting a tab. I added a indent rule such that aligns a
> line in a string block to the previous line, for the first line, it indents one level.
>
> 5. That’s hard, if not impossible, for tree-sitter to do. When using a
> parser for highlight/indentation, you need provide a correct (or
> close to correct) source code for it to work right. That’s part of
> the trade-off comparing to regexp-based highlighting.
>
> 6. For the parameter indentation, I recently added
> c-ts-common-baseline-indent-rules that can handle it correctly. So
> if we remove the existing indent rule for the parameters and add
> c-ts-common-baseline-indent-rules at the end so the indentation
> falls back to it, the indentation would look like this:
>
> public class TextBlocks {
> public record StudentRecord(String firstName,
> String lastName,
> Long studentId,
> String email) {
>
> }
>
>
> public String filterData(@RequestParam(required = false) String name,
> @RequestParam(required = false) String name,
> @RequestParam(required = false) Integer age
> )
> }
>
> Seems fair? Theo, WDYT?
>
> > - Annotations (@Annotations)
> It seems to work fine? What’s the problem that you see?
>
> > - Diamond Brackets (<>)
> Same as above, what’s the desired behavior?
>
> > - Constants, Static Variables, Enum Variables should be highlighted with
> > distinct colors and optionally italic font
>
> For constants, they aren’t highlighted in constant face because rules
> for ‘definition’ and ‘expression’ feature overrides them with
> variable-name face. We can fix this by either moving the ‘constant’
> face after ‘definition’ and ‘expression’ feature, or remove the
> :override flag for ‘definition’ and ‘expression’ feature. Theo, any
> suggestions here?
>
> > - Unused Variables or Classes (Grayed Out)
> > - Unused variables, unused classes, etc., highlighted in gray. Not
> sure if this can be achieved
>
> Tree-sitter can’t do this. So the only option is to use eglot for it.
>
> Finally, some suggestions on communication. As you said on reddit,
> you’re not a native English speaker, so I can’t blame you, but some
> minor changes to wording can make your message sound a lot kinder :)
> For example, short and imperative sentences like "you understand?"
> sounds harsh and condescending; OTOH something like "I hope you can
> understand/get that..." is a lot better. As a rule of thumb, the less
> certain and the longer your expression is, the softer it sounds ;-)
>
> Yuan
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.