elm-tooling / tree-sitter-elm

Tree sitter implementation for elm
https://elm-tooling.github.io/tree-sitter-elm/
MIT License
74 stars 12 forks source link

Fix end line comment not being included in node text #144

Closed Qluxzz closed 9 months ago

Qluxzz commented 9 months ago
type Foo
    = Bar -- This is a Bar

Currently when hovering over Foo in VS Code the result is this:

type Foo
    = Bar

The ending line comment is dropped.

However for types with multiple variants, such as the following:

type Foo
    = Bar -- This is a Bar
    | Biz -- This is a Biz

The result when hovering is the following:

type Foo
    = Bar -- This is a Bar
    | Biz

The first variant's line comment is included, but not the last one.

The following changes makes it so the ending line comment is included in the correct node text.

I've also verified that these changes have the correct behaviour in the elm-language-server repo with the following tests added to test/hoverProvider.test.ts and using a locally built tree-sitter-elm.wasm version:

  it("should include line comment when hovering", async () => {
    const source = `
--@ Test.elm
module Test exposing (..)

type Foo
    = Bar -- This is a Bar

bar : Foo
     --^
bar = Bar
  `;

    await testHover(
      source,
      "```elm\ntype Foo\n    = Bar -- This is a Bar\n```",
    );
  });

  it("should include end line comment when hovering", async () => {
    const source = `
  --@ Test.elm
module Test exposing (..)

type Foo
    = Bar -- This is a Bar
    | Biz -- This is a Biz

bar : Foo
     --^
bar = Bar
  `;

    await testHover(
      source,
      "```elm\ntype Foo\n    = Bar -- This is a Bar\n    | Biz -- This is a Biz\n```",
    );
  });
razzeee commented 9 months ago

Thank you, seems sane to me. I will probably squash this when merging.

Qluxzz commented 9 months ago

Yeah, go right ahead and squash, I naively assumed that squashing was the default merging strategy.

razzeee commented 9 months ago

Unfortunatly this regresses a bit

With your MR

Total parses: 20011; successful parses: 19930; failed parses: 81; success percentage: 99.60% 

Main branch

Total parses: 20011; successful parses: 19938; failed parses: 73; success percentage: 99.64% 

I guess we should check those 8 cases

Qluxzz commented 9 months ago

I diffed the log files and these are the new regressions.

examples-full/Janiczek/elm-graph/src/Graph.elm                           1 ms  (ERROR [70, 8] - [77, 9])
examples-full/Kurren123/k-dropdown-container/src/DropdownContainer.elm   0 ms  (ERROR [63, 13] - [63, 16])
examples-full/MacCASOutreach/graphicsvg/src/GraphicSVG.elm               11 ms (ERROR [514, 4] - [514, 7])
examples-full/arowM/elm-neat-layout/src/Neat/Internal.elm                1 ms  (ERROR [64, 4] - [64, 7])
examples-full/evancz/elm-playground/src/Playground.elm                   4 ms  (ERROR [828, 2] - [828, 5])
examples-full/folkertdev/elm-brotli/src/RingBuffer.elm                   1 ms  (ERROR [19, 21] - [19, 24])
examples-full/jeongoon/elmnt-scrollpicker/src/Elmnt/BaseScrollPicker.elm 6 ms  (ERROR [248, 32] - [248, 47])
examples-full/pfcoperez/elm-playground/src/Playground.elm                4 ms  (ERROR [828, 2] - [828, 5])
Qluxzz commented 9 months ago

I compared the new regressions to the main branch and these new tests catch the regression:

================================================================================
Type declaration with union variant and associated data and line comment on new line
================================================================================

type Foo
    = Bar
        -- First associated data
        Int

--------------------------------------------------------------------------------

(file
      (type_declaration
        (type)
        (upper_case_identifier)
        (eq)
        (union_variant
          (upper_case_identifier)
          (line_comment)
          (type_ref
            (upper_case_qid
              (upper_case_identifier))))))

================================================================================
Type declaration with union variant and associated data with line comment on same line
================================================================================

type Foo
    = Bar
        Int -- First associated data
        Float -- Second associated data

--------------------------------------------------------------------------------

(file
      (type_declaration
        (type)
        (upper_case_identifier)
        (eq)
        (union_variant
          (upper_case_identifier)
          (type_ref
            (upper_case_qid
              (upper_case_identifier)))
          (line_comment)
          (type_ref
            (upper_case_qid
              (upper_case_identifier))))
          (line_comment)))

I'm currently working on a fix.

Qluxzz commented 9 months ago

Running npm run test-full with the latest changes does now not report any regressions, compared to the main branch.

Total parses: 20011; successful parses: 19938; failed parses: 73; success percentage: 99.64%

razzeee commented 9 months ago

LGTM, let's see if this still works in the server