Update: changed curly reporting location (refs #12334)

[anikethsaha]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Updated the location of reporting for curly braces.

Currently, the column while reporting was always 1 except for else.

I made the following changes.

• for missing parens, it will show the location where is should be placed.
  for example

1 for(;;)
2 foo();
  ^ <- this location will be reported.

or

if(foo) bar;
    // ^ <- this location will be reported

• for unexpected braces,
  the first unexpected { will be reported.

if(foo) {foo()};
     // ^ 

The else logic for both is not changed as I think it was working fine earlier

Is there anything you'd like reviewers to focus on?

None

[anikethsaha]

I am not sure what am I missing in CI cause the failing test is un-related to my changes and tests are passing in my machine.

any suggestions would be appreciated 👍

[mdjermanovic]

Those two failing tests should be fixed by #13277.

[kaicataldo]

@anikethsaha Do you mind rebasing now that #13277 has been merged?

[kaicataldo]

Thanks for working on this!

[mdjermanovic]

Locations for missing { seem to be inconsistent:

/*eslint curly: ["error"]*/

    if (a)
//        ^  
        b()

    else
//  ^
        c();

    for (d of e)
        f();
//      ^  

Maybe we should discuss first what would be reasonable to report?

[anikethsaha]

Should all of them have the starting location of the body ?

[mdjermanovic]

Should all of them have the starting location of the body ?

That would be reasonable given that it marks the place where the auto-fix insert {.

On the other hand, editors usually don't show zero-width ranges well. Also, user might want to insert { at a different place, and the message says "Expected { after...".

Maybe we could instead consider reporting the location of the first token (which would be if, else, while, do, or for):

if (foo) bar();
^^

or the last token before the body (which would be ), else or do):

if (foo) bar();
       ^      

or maybe even the whole range before the body (though, this could underline a lot of code):

if (foo) bar();
^^^^^^^^      

[anikethsaha]

Maybe we could instead consider reporting the location of the first token (which would be if, else, while, do, or for):

if (foo) bar();
^^

I think this is the current behavior, it shows the column as 1

or the last token before the body (which would be ), else or do):

if (foo) bar();
       ^      

let me try this,

or maybe even the whole range before the body (though, this could underline a lot of code):

if (foo) bar();
^^^^^^^^      

true, it can be longer if the condition inside the is long.

[mdjermanovic]

Maybe we could instead consider reporting the location of the first token (which would be if, else, while, do, or for):

if (foo) bar();
^^

I think this is the current behavior, it shows the column as 1

It is the current behavior for start location, but it doesn't report end location.

If we choose this option, it might make sense to report the whole range of the first token (i.e., both its start and end locations).

[anikethsaha]

Update :

Now, the reporting location is as follows.

for missing

if (foo) bar();
    // ^^

if (foo) 
    // ^^   
 bar();
    

// `else`

if (foo) bar(); else bar(1);
    //          ^^^^


// `for`

for (;;) bar();
//     ^^

for unexpected

if (foo) {bar();}
   //    ^^

// `else`

if (foo) bar(); else {bar(1);}
    //               ^^

for (;;) {bar()};
//       ^^

@mdjermanovic let me know if this is ok !

[mdjermanovic]

@mdjermanovic let me know if this is ok !

I'm not sure which of the three solutions I suggested would be the most appropriate, or if there's another one I didn't think of.

@kaicataldo thoughts?

[kaicataldo]

Could it make sense to always report the locations between the last token before the body and the first token of the body, regardless of whether it's missing or not?

Missing:

if (foo) bar();
       ^^^

if (foo) 
       ^^   
 bar();
^^ 

if (foo) bar(); else bar(1);
       ^^^         ^^^

for (;;) bar();
       ^^^

Unexpected:

if (foo) {bar();}
       ^^^^

if (foo) bar(); else {bar(1);}
                   ^^^^

for (;;) {bar()};
       ^^^^

[anikethsaha]

I think that would make sense for missing case.
For Unexpected case, I think it should report the curly brace itself.

WDYT ?

[kaicataldo]

That makes sense to me!

[anikethsaha]

I have made the changes as discussed above.

Let me know if anything else if required

[snitin315]

I will finish it.

[snitin315]

Stale. Closing in favor of #14766