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

feature request: Include source code location for each node #21

Closed
maxh opened this issue Jun 12, 2023 · 9 comments · Fixed by #25
Closed

feature request: Include source code location for each node #21

maxh opened this issue Jun 12, 2023 · 9 comments · Fixed by #25

Comments

@maxh
Copy link
Contributor

maxh commented Jun 12, 2023

I'm using your library to power https://github.com/loop-payments/prisma-lint. It's working great -- thanks!

I'd like to show RuboCop-like errors with the exact source text embedded, maybe something like this:

https://github.com/rubocop/rubocop/blob/c3c9b42b876d08d83738d57203c0baf9b4b0b865/spec/rubocop/cop/style/unless_else_spec.rb#L7-L8

Having location within the source code for each node would be useful. Perhaps something like what ESLint does:

https://github.com/eslint/eslint/blob/e0cf0d86d985ed2b2f901dd9aab5ccd2fff062ad/tests/lib/rules/array-callback-return.js#L458-L461

What do you think?

@maxh maxh changed the title feature request: Include source source code location each node feature request: Include source code location each node Jun 12, 2023
@maxh maxh changed the title feature request: Include source code location each node feature request: Include source code location for each node Jun 12, 2023
@MrLeebo
Copy link
Owner

MrLeebo commented Jun 13, 2023

It sounds intriguing. prisma-ast is built with chevrotain.js, but I'll admit I don't really know that much about building syntax trees and mostly just stumbled my way through this project. It produces an AST because they were simpler to implement than CSTs.

I believe chevrotain does support location info for tokens, so it would be possible to surface that info.

What I'd like to do is have a better API for querying the document, but until now I never really had a good idea for the sorts of use cases an API like that would need to serve. I'm open to your suggestions on what kinds of features a prisma-ast Query interface would be most helpful to provide.

@maxh
Copy link
Contributor Author

maxh commented Jun 15, 2023

I believe chevrotain does support location info for tokens, so it would be possible to surface that info

Makes sense! Let me look into this a little bit and circle back. I haven't thought too deeply beyond the idea of mirroring ESLint and RuboCop developer experience.

What I'd like to do is have a better API for querying the document, but until now I never really had a good idea for the sorts of use cases an API like that would need to serve.

The visitor pattern implemented in the linter works well for traversal:

https://github.com/loop-payments/prisma-lint/blob/7cc13d728ac850ce675b1aeac4e6d13a614f83d6/src/lint-prisma-source-code.ts#L41-L61

But maybe there is a better approach, or other use cases for a query API.

@MrLeebo
Copy link
Owner

MrLeebo commented Jul 8, 2023

@maxh I've created a PR with the option to enable node location tracking. It's only sort of partially implemented, to fit in with the way the schema is currently shaped, but perhaps it will be enough for your use cases.

Can you try it out and see if it works for your usage?

Location tracking is disabled by default, since there is an impact on parsing performance, but you can turn it on by adding a configuration option to your package.json file.

{
  "prisma-ast": {
    "parser": {
      "nodeLocationTracking": "full"
    }
  }
}

After that, "location" statistics will appear on certain parts of your schema output from getSchema(). The available statistics will appear as such:

{
    type: 'generator',
    name: 'client',
    assignments: [...],
    location: {
      startLine: 7,
      startColumn: 1,
      startOffset: 79,
      endLine: 7,
      endColumn: 16,
      endOffset: 94
    }
  }

Hopefully that will be sufficient for your linter.

@maxh
Copy link
Contributor Author

maxh commented Jul 9, 2023

Looks promising! I'm testing it out here:

loop-payments/prisma-lint#84

But running into problems with types; not sure if your side or mine:

There are types at '/home/runner/work/prisma-lint/prisma-lint/node_modules/@mrleebo/prisma-ast/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The '@mrleebo/prisma-ast' library may need to update its package.json or typings.

https://github.com/loop-payments/prisma-lint/actions/runs/5502248068/jobs/10026376231?pr=84

@MrLeebo
Copy link
Owner

MrLeebo commented Jul 10, 2023

@maxh Please try again.

I had added a "exports" section to the package.json, but I guess it broke the TypeScript typings so I rolled it back.

@maxh
Copy link
Contributor Author

maxh commented Jul 14, 2023

Ok, it is looking good:

loop-payments/prisma-lint#84

In the test scenario, I have it printing:

model Users {
^^^^^^^^^^^

It seems that model nodes have location nodes but field nodes do not. Is that expected? I think ideally if there is a field-level violation we can underline that field specifically.

Given that the variance in position and context for model names and field names is pretty standardized (models are top-level nodes, fields are nested within them), I'm second-guessing whether it makes to mirror ESLint and RuboCop for the CLI rendered output of the linter.

But there are other use cases for these locations -- for example an LSP implementation for highlighting errors inside an editor.

@MrLeebo
Copy link
Owner

MrLeebo commented Jul 16, 2023

Fixed the presence of location data for field and attribute node types.

@maxh
Copy link
Contributor Author

maxh commented Jul 18, 2023

Looking ready to go! One small thing -- I wasn't able to import the CstNodeLocation type when annotating a function:

https://github.com/loop-payments/prisma-lint/pull/84/files#r1266591956

Otherwise LGTM. I have another library in the pipeline this will be perfect for too. Thanks!!

@MrLeebo
Copy link
Owner

MrLeebo commented Jul 19, 2023

Added CstNodeLocation to the export, but it is just a pass-thru from chevrotain, so you can also get it from there.

import type { CstNodeLocation } from '@mrleebo/prisma-ast';
// is the same as
import type { CstNodeLocation } from 'chevrotain';

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