Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infix multiplication operator screws up parentheses coloring and is highlighted in red #209

Open
mbottini opened this issue Dec 21, 2023 · 10 comments · May be fixed by #210
Open

Infix multiplication operator screws up parentheses coloring and is highlighted in red #209

mbottini opened this issue Dec 21, 2023 · 10 comments · May be fixed by #210

Comments

@mbottini
Copy link
Contributor

mbottini commented Dec 21, 2023

Describe the bug
First, the infix multiplication operator (*) is highlighted in red, differently from the other operators.

image

And second, likely as a result of not being captured as a parenthesized operator, the opening paren of the operator is not counted, and the closing paren is counted for the purposes of matching corresponding parentheses. In this screenshot, the closing paren of the operator is matched with the parenthesis before Seq.fold:

image

To Reproduce
Steps to reproduce the behaviour:

  1. Open up a new file and type (*) to show the red-highlighted operator.

      (*)
    
  2. Call the (*) operator inside of another parenthesized expression, which will cause the operator's closing paren to match with the opening paren of the outer expression.

     (Seq.fold (*) 1 [1;2;3])
    

Expected behaviour
The (*) operator should be treated identically to the other operators. At a minimum, it should not affect the parenthesis coloring of outer expressions.

Environment (please complete the following information):

  • OS: Any
  • Ionide version: v7.16.1
  • VSCode version: 1.85.1
  • dotnet SDK version: 8.0.100
  • mono / .Net Framework version: (N/A)
@mbottini
Copy link
Contributor Author

It appears to me that this is correctly captured by the grammar. See the following comparisons between (*) and (+) with regard to what the Scope Inspector has to say. They're the same.

image

image

Note that the color (which should be something like #c21c1a )doesn't line up. Something else is formatting the paren color this way.

@mbottini
Copy link
Contributor Author

This did it. I done goofed - Ionide has nothing to do with this. This is a VSCode bug, I'll go bug them.

image

@mbottini
Copy link
Contributor Author

After a bunch of work to download and compile VSCode and adding the updated grammar file, the issue persists with the bracket pair colorization. There's something in the grammar that's making it unhappy with the parens. I'll keep digging!

@mbottini mbottini reopened this Dec 22, 2023
@mbottini
Copy link
Contributor Author

mbottini commented Dec 22, 2023

Found it - had to dig through VSCode's source for how it's doing its bracket colorization.

fsharp.syntaxtest/language-configuration.json defines extra bracket pairs from what is in the base VSCode extension package. Here's what VSCode has:

"brackets": [
    ["{", "}"],
    ["[", "]"],
    ["(", ")"]
]

And here's what Ionide has:

"brackets": [
   ["(", ")"],
   ["(*", "*)"],
   ["{", "}"],
   ["[", "]"],
   ["[|", "|]"],
   ["<@", "@>"],
   ["<@@", "@@>"]
}

Should the block comments be in there? They usually aren't in there for other languages.

@mbottini
Copy link
Contributor Author

Sure enough, removing ["(*", "*)"], from the "brackets" field fixes it.

image

I'll put in the pull request - other folks can opine on whether this is a good idea or not.

@MangelMaxime
Copy link
Contributor

I think the block comments are in there to support automatic closing of the "brackets".

CleanShot.2023-12-22.at.09.53.48.mp4

You can spot an highlighting error in my video 🙈

@mbottini
Copy link
Contributor Author

mbottini commented Dec 22, 2023

I think the block comments are in there to support automatic closing of the "brackets".
CleanShot.2023-12-22.at.09.53.48.mp4

You can spot an highlighting error in my video 🙈

That's a separate field (autoClosingPairs) in the language_configuration.json file, which I haven't touched. Similarly, surroundingPairs allows you to highlight a selection and then enclose it, but it doesn't seem to work with (* *) whether it's in the brackets field or not. That might be another issue for me to try to chase down!

mbottini added a commit to mbottini/ionide-fsgrammar that referenced this issue Dec 22, 2023
language-configuration.json lets you define various pairs of brackets
and braces for autoclosing, surrounding selected text with characters,
and, as it turns out, dictating which brackets get colorized by
the native bracket colorizer.

As far as I can tell, putting the block comment brackets inside the
`brackets` field is pointless. We color our comments green anyway,
so the colors don't show anyway. It is true that F# provides
support for nested comment blocks, but we aren't taking advantage
of the color feature anyway (and I'm unsure if it's even possible
to do so).

The block comments should, however, remain in the other fields
that control autocomplete and surrounding selected text.
mbottini added a commit to mbottini/ionide-fsgrammar that referenced this issue Dec 22, 2023
language-configuration.json lets you define various pairs of brackets
and braces for autoclosing, surrounding selected text with characters,
and, as it turns out, dictating which brackets get colorized by
the native bracket colorizer.

As far as I can tell, putting the block comment brackets inside the
`brackets` field is pointless. We color our comments green anyway,
so the colors don't show anyway. It is true that F# provides
support for nested comment blocks, but we aren't taking advantage
of the color feature anyway (and I'm unsure if it's even possible
to do so).

The block comments should, however, remain in the other fields
that control autocomplete and surrounding selected text.
@mbottini mbottini linked a pull request Dec 22, 2023 that will close this issue
@MangelMaxime
Copy link
Contributor

@mbottini Thank you for the confirmation.

mbottini added a commit to mbottini/ionide-fsgrammar that referenced this issue Dec 22, 2023
language-configuration.json lets you define various pairs of brackets
and braces for autoclosing, surrounding selected text with characters,
and, as it turns out, dictating which brackets get colorized by
the native bracket colorizer.

Currently, having `(* *)` inside the `brackets` field is causing the
infix multiplication operator to be treated by the bracket colorizer
as two brackets: an opening `(*` and a closing `)`. Two things happen
here:

* The `(*` does not correspond to a corresponding `*)` bracket, so it
  is treated as unbalanced (hence why that part of the operator is
  colored red).
* Absent any preceding parens in the expression, the `)` is also
  treated as an unbalanced paren and is colored red. This is the
  best case, which colors the entire operator a uniform red. Strange,
  but normal-looking enough that it actually looked intentional to me
  when I first started getting into F#. The multiplication operator is
  weird due to how similar it is to block comments - maybe it's
  supposed to be that way!
* But the bug really gets exposed when there is a preceding paren in
  the expression such as in the expression `(Seq.fold (*) 1 [1;2;3])`.
  In this case, there *is* a balancing paren - the paren that precedes
  `Seq.fold`. Now the multiplication operator is half red (the block
  comment bracket never gets balanced) and half whatever the colorizer
  picks for the two parens. And now the closing paren after `[1;2;3]` is
  unbalanced and made red!

As far as I can tell, putting the block comment brackets inside the
`brackets` field is pointless. We color our comments green anyway,
so the colors don't show anyway. It is true that F# provides
support for nested comment blocks, but we aren't taking advantage
of the color feature anyway (and I'm unsure if it's even possible
to do so).

The block comments should, however, remain in the other fields
that control autocomplete and surrounding selected text.
@mbottini
Copy link
Contributor Author

You can spot an highlighting error in my video 🙈

I missed this part - that's interesting. Consider that when apart, VSCode handles them correctly:

image

But when they're together, VSCode's bracket balancer greedily matches on <@@ and complains that it's unbalanced. I should go bug them about that - it's a subtle edge case that they should be able to test for.

mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the (* *) bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the *) on the next line
with the same indentation level as the (* as follows:

    (*
        █
    *)

when I hit Enter with an unaccompanied (*:

    (*█

It should simply indent as follows:

    (*
        █

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

when I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

---

When I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

---

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

---

When I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

---

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

---

When I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

---

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209 and ionide/ionide-fsgrammar#210

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

---

When I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

---

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209 and ionide/ionide-fsgrammar#210

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair
from the "brackets" field of language-configuration.json. One issue
with doing this is that we lose the bracket-like indentation that
VSCode provides by default for all bracket pairs.

Since I'd like to remove the block comment brackets from the
`language-configuration.json` file, this pull request re-adds
the same semantics to the indentation rules.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

---

When I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

---

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
mbottini added a commit to mbottini/ionide-vscode-fsharp that referenced this issue Apr 1, 2024
Background: ionide/ionide-fsgrammar#209 and ionide/ionide-fsgrammar#210

This comes from a comment regarding a pull request that I made on the
ionide-fsgrammar repository, which removes the `(* *)` bracket pair from
the "brackets" field of language-configuration.json. One issue with
doing this is that we lose the bracket-like indentation that VSCode
provides by default for all bracket pairs. This pull request re-adds the
same semantics to the indentation rules.

In other words, when I hit Enter in the following configuration,
cursor location represented by the white block:

    (*█*)

It should indent the cursor and then put the `*)` on the next line
with the same indentation level as the `(*` as follows:

    (*
        █
    *)

---

When I hit Enter with an unaccompanied `(*`:

    (*█

It should simply indent as follows:

    (*
        █

---

Lastly, an unaccompanied `*)` should outdent. That is,

        *)█

should become

    *)█
@mbottini
Copy link
Contributor Author

mbottini commented Apr 1, 2024

Oh no, I didn't realize that all of my edits to the commit message were going to get shown here! I was using the pull request window to render the Markdown! Whoops. Maybe a maintainer can collapse all of those commits...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants