Subject: Re: bug#75154: 31.0.50; java-ts-mode. Issues with Indentation

From: Yuan Fu <[email-masked]>
Date: Thu, 13 Feb 2025 22:24:06 -0800

> On Jan 28, 2025, at 6:06 AM, Artem <[email-masked]> 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