rescript-lang / rescript-vscode

Official VSCode plugin for ReScript
MIT License
329 stars 56 forks source link

add `moduletypeid` field for modules with explicitly annotated module type #1019

Closed aspeddro closed 4 months ago

zth commented 4 months ago

Perhaps it'd be better to have both the module type name and the module type id?

woeps commented 4 months ago

Looks good to me! :D

The only possible issue I found is, when the explicitly annotated module type is in another file:

// Types.res
module type T = {
  let x: int
}
// Example.res
module M: Types.T = {
  let x = 42
}

Above will result in moduletypeid=Example.Types.T in the json generated for module M. I would expect the id to be Types.T.

PS: module types in the same file are working as expected.

woeps commented 4 months ago

Perhaps it'd be better to have both the module type name and the module type id?

Tbh, I don't think the name is necessary in the light of progrmatically processing the data to e.g. build a docs website. In the sense of a "normalized dataset", I just need the reference on where to find the details.

But if you (@zth) have a usecase in mind, where the name would be convenient to have without further id resolutions, I'm not opposed. I'm just saying I dont't necessary need the name for the usecases I'm thinking about.

zth commented 4 months ago

No I don't have anything specific in mind really. Just a thought.

Is this ready to go?

aspeddro commented 4 months ago

No, I'll take a look later

aspeddro commented 4 months ago

Right, moduletypeid fixed.

Example.res

module type T = {
  let a: int
}
module M: Types.T = {
  let x = 42
}
module A: T = {
  let a = 1
}

Types.res

module type T = {
  let x: int
}
{
  "name": "Example",
  "docstrings": [],
  "source": {
    "filepath": "src/Example.res",
    "line": 1,
    "col": 1
  },
  "items": [
  {
    "id": "Example.T",
    "name": "T",
    "kind": "moduleType",
    "docstrings": [],
    "source": {
      "filepath": "src/Example.res",
      "line": 1,
      "col": 13
    },
    "items": [
    {
      "id": "Example.T.a",
      "kind": "value",
      "name": "a",
      "signature": "let a: int",
      "docstrings": [],
      "source": {
        "filepath": "src/Example.res",
        "line": 2,
        "col": 3
      }
    }]
  }, 
  {
    "id": "Example.M",
    "name": "M",
    "kind": "module",
    "moduletypeid": "Types.T",
    "docstrings": [],
    "source": {
      "filepath": "src/Example.res",
      "line": 1,
      "col": 1
    },
    "items": [
    {
      "id": "Example.M.x",
      "kind": "value",
      "name": "x",
      "signature": "let x: int",
      "docstrings": [],
      "source": {
        "filepath": "src/Example.res",
        "line": 5,
        "col": 7
      }
    }]
  }, 
  {
    "id": "Example.A",
    "name": "A",
    "kind": "module",
    "moduletypeid": "T",
    "docstrings": [],
    "source": {
      "filepath": "src/Example.res",
      "line": 1,
      "col": 1
    },
    "items": [
    {
      "id": "Example.A.a",
      "kind": "value",
      "name": "a",
      "signature": "let a: int",
      "docstrings": [],
      "source": {
        "filepath": "src/Example.res",
        "line": 8,
        "col": 7
      }
    }]
  }]
}
aspeddro commented 4 months ago

Ready to go!

woeps commented 4 months ago

Right, moduletypeid fixed.

Example.res

module type T = {
  let a: int
}
module M: Types.T = {
  let x = 42
}
module A: T = {
  let a = 1
}

Types.res

module type T = {
  let x: int
}

Sorry to be a downer 🫣: I believe the generated moduletype for module A should be Example.T. (in the given Example the filename Example is missing)

I'll check if this is just a typo in the comments or the actual implementation in a bit.

woeps commented 4 months ago

I just verified: the json example in the comment above from @aspeddro is according to the implementation.

To me, it seems it is a little bit more complicated than just passing modulepath to makeId or not.

My expectation of the generated moduletypeid when a module type is explictly annotated inside a file Example.resi: <FileName>.<PathInFile>.<ModuleTypeName>

zth commented 4 months ago

@woeps I interpret this as the PR isn't ready to go just yet?

woeps commented 4 months ago

@woeps I interpret this as the PR isn't ready to go just yet?

@zth as I see it, no. This isn't ready yet.

aspeddro commented 4 months ago

Ok, I think is this, right?

{
  "name": "Example",
  "docstrings": [],
  "source": {
    "filepath": "src/Example.res",
    "line": 1,
    "col": 1
  },
  "items": [
  {
    "id": "Example.T",
    "name": "T",
    "kind": "moduleType",
    "docstrings": [],
    "source": {
      "filepath": "src/Example.res",
      "line": 1,
      "col": 13
    },
    "items": [
    {
      "id": "Example.T.a",
      "kind": "value",
      "name": "a",
      "signature": "let a: int",
      "docstrings": [],
      "source": {
        "filepath": "src/Example.res",
        "line": 2,
        "col": 3
      }
    }]
  }, 
  {
    "id": "Example.M",
    "name": "M",
    "kind": "module",
    "moduletypeid": "Types.T",
    "docstrings": [],
    "source": {
      "filepath": "src/Example.res",
      "line": 1,
      "col": 1
    },
    "items": [
    {
      "id": "Example.M.x",
      "kind": "value",
      "name": "x",
      "signature": "let x: int",
      "docstrings": [],
      "source": {
        "filepath": "src/Example.res",
        "line": 6,
        "col": 7
      }
    }]
  }, 
  {
    "id": "Example.A",
    "name": "A",
    "kind": "module",
    "moduletypeid": "Example.T",
    "docstrings": [],
    "source": {
      "filepath": "src/Example.res",
      "line": 1,
      "col": 1
    },
    "items": [
    {
      "id": "Example.A.a",
      "kind": "value",
      "name": "a",
      "signature": "let a: int",
      "docstrings": [],
      "source": {
        "filepath": "src/Example.res",
        "line": 10,
        "col": 7
      }
    }]
  }]
}
woeps commented 4 months ago

@aspeddro great progress! :+1:

I found one more case, which needs to be handled (when submodules have explicit module types):

// Example.res
module type MT = {
  let x: int
}

module A: MT = {
  let x = 42
}

module B: Types.T = {
  let x = 42
}

module C = {
  module D: MT = // generates "moduletypeid": "Example.C.MT" - but should be Example.MT
    let x = 42
  }
  module E: Types.T = {
    let x = 3
  }
}

generates this json:

{
  "name": "Example",
  "docstrings": [],
  "source": {
    "filepath": "test/Example.res",
    "line": 1,
    "col": 1
  },
  "items": [
  {
    "id": "Example.MT",
    "name": "MT",
    "kind": "moduleType",
    "docstrings": [],
    "source": {
      "filepath": "test/Example.res",
      "line": 1,
      "col": 13
    },
    "items": [
    {
      "id": "Example.MT.x",
      "kind": "value",
      "name": "x",
      "signature": "let x: int",
      "docstrings": [],
      "source": {
        "filepath": "test/Example.res",
        "line": 2,
        "col": 3
      }
    }]
  }, 
  {
    "id": "Example.A",
    "name": "A",
    "kind": "module",
    "moduletypeid": "Example.MT",
    "docstrings": [],
    "source": {
      "filepath": "test/Example.res",
      "line": 1,
      "col": 1
    },
    "items": [
    {
      "id": "Example.A.x",
      "kind": "value",
      "name": "x",
      "signature": "let x: int",
      "docstrings": [],
      "source": {
        "filepath": "test/Example.res",
        "line": 6,
        "col": 7
      }
    }]
  }, 
  {
    "id": "Example.B",
    "name": "B",
    "kind": "module",
    "moduletypeid": "Types.T",
    "docstrings": [],
    "source": {
      "filepath": "test/Example.res",
      "line": 1,
      "col": 1
    },
    "items": [
    {
      "id": "Example.B.x",
      "kind": "value",
      "name": "x",
      "signature": "let x: int",
      "docstrings": [],
      "source": {
        "filepath": "test/Example.res",
        "line": 10,
        "col": 7
      }
    }]
  }, 
  {
    "id": "Example.C",
    "name": "C",
    "kind": "module",
    "docstrings": [],
    "source": {
      "filepath": "test/Example.res",
      "line": 13,
      "col": 8
    },
    "items": [
    {
      "id": "Example.C.D",
      "name": "D",
      "kind": "module",
      "moduletypeid": "Example.C.MT",  <-- this should be Example.MT
      "docstrings": [],
      "source": {
        "filepath": "test/Example.res",
        "line": 1,
        "col": 1
      },
      "items": [
      {
        "id": "Example.C.D.x",
        "kind": "value",
        "name": "x",
        "signature": "let x: int",
        "docstrings": [],
        "source": {
          "filepath": "test/Example.res",
          "line": 15,
          "col": 9
        }
      }]
    }, 
    {
      "id": "Example.C.E",
      "name": "E",
      "kind": "module",
      "moduletypeid": "Types.T",
      "docstrings": [],
      "source": {
        "filepath": "test/Example.res",
        "line": 1,
        "col": 1
      },
      "items": [
      {
        "id": "Example.C.E.x",
        "kind": "value",
        "name": "x",
        "signature": "let x: int",
        "docstrings": [],
        "source": {
          "filepath": "test/Example.res",
          "line": 18,
          "col": 9
        }
      }]
    }]
  }]
}

on a side note: It seems source for module A, B, D & E is incorrect: I'll create a new issue, once this pr is merged.

zth commented 4 months ago

Thanks for both of your work! Keep it going! 😄

aspeddro commented 4 months ago

Added some tests https://github.com/rescript-lang/rescript-vscode/pull/1019/commits/a40df41b5abf0bca962834facfe7444b6d277590

woeps commented 4 months ago

@zth I think this is now good to be merged.

zth commented 4 months ago

Great work! Do you want me to put out another tools version with this or is there other outstanding things we should try and fix before?

woeps commented 4 months ago

@zth You may publish a new tools version. This PR stands on its own and is a great step forward. 🥳

zth commented 4 months ago

@woeps 0.6.4 is on its way out.