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

Signature help shows wrong active parameter for methods with type params and empty param list. #19969

Closed
rochala opened this issue Mar 18, 2024 · 4 comments · Fixed by #20142
Closed
Labels
area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools good first issue Perfect for someone who wants to get started contributing itype:bug Spree Suitable for a future Spree
Milestone

Comments

@rochala
Copy link
Contributor

rochala commented Mar 18, 2024

Compiler version

3.4.1

Minimized code

  // file dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala
  @Test def `proper-param-empty-list` =
    check(
      """
        |object x {
        |  def foo[K, V](): Unit = ???
        |  foo(@@)
        |}
        |""".stripMargin,
      "foo[K, V](): Unit"
    )

Output

Last parameter is selected as current argument.

Expectation

No element should be selected as active parameter.

@rochala rochala added itype:bug good first issue Perfect for someone who wants to get started contributing Spree Suitable for a future Spree area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools labels Mar 18, 2024
@mbovel
Copy link
Member

mbovel commented Apr 8, 2024

This issue was picked for the Scala Issue Spree of tomorrow, April 9th. @rochala, @mbovel, @iusildra, @nmcb will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@mbovel
Copy link
Member

mbovel commented Apr 9, 2024

Rust-analyzer behavior for comparison, it shows an index higher than the maximum parameter index:

Result: {
    "signatures": [
        {
            "label": "fn foo(_x: i32)",
            "parameters": [
                {
                    "label": [
                        7,
                        14
                    ]
                }
            ],
            "activeParameter": 1
        }
    ],
    "activeSignature": 0,
    "activeParameter": 1
}

Where src/bin/hello.rst is:

fn foo<T>(_x: i32) {
}

fn main() {
    foo::<i32>(32, **CURSOR_HERE**);
    println!("Hello, world!");
}

And cargo.toml is:

[workspace]

[package]
name = "hello" # the name of the package
version = "0.1.0"    # the current version, obeying semver
authors = ["Alice <[email protected]>", "Bob <[email protected]>"]

[[bin]]
name = "hello"

Screenshots:

image image

@mbovel
Copy link
Member

mbovel commented Apr 10, 2024

For context and for the record, we observed during the spree that the language server protocol currently does not provide a way to indicate that no parameter is active. Instead, the specification says that any parameter outside of the range will default to 0:

        /**
	 * The active parameter of the active signature. If omitted or the value
	 * lies outside the range of `signatures[activeSignature].parameters`
	 * defaults to 0 if the active signature has parameters.  [...]
	 */
	activeParameter?: uinteger;

However, we observed that VSCode does not follow this and will actually not highlight any parameter if activeParameter is outside of the range, and it is the case for the Rust LSP integration shown in my comment above.

Tracking LSP issue: microsoft/language-server-protocol#1271. In the future, null might be allowed as a value for activeParameter to explicitly mean that no parameter should be highlighted.

@rochala
Copy link
Contributor Author

rochala commented Apr 11, 2024

I've started tracking the upstream issue in language-server-protocol. If there is any update in the future, I'll make required changes / create a new ticket.

@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
WojciechMazur pushed a commit that referenced this issue Jul 5, 2024
Fixes #19969 with @mbovel @rochala

---------

Co-authored-by: Lucas Nouguier <[email protected]>
[Cherry-picked adf089b]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools good first issue Perfect for someone who wants to get started contributing itype:bug Spree Suitable for a future Spree
Projects
None yet
3 participants