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

ParserError is missing end position #156

Open
tscpp opened this issue Nov 15, 2020 · 16 comments
Open

ParserError is missing end position #156

tscpp opened this issue Nov 15, 2020 · 16 comments

Comments

@tscpp
Copy link

tscpp commented Nov 15, 2020

I need the end position of any parser error for my linter, but this is not an available property in the ParserError class.

@3cp
Copy link
Member

3cp commented Nov 15, 2020

@KFlash this is probably going to be a breaking change.
Changing loc: {line, column} to loc: {start: {line, column}, end: {line, column}}.

@KFlash
Copy link
Contributor

KFlash commented Nov 15, 2020

What other parsers do ?

@3cp
Copy link
Member

3cp commented Nov 15, 2020

I see your point. It seems babel/parser also has the loc: {line, column}.

{
  "loc": {
    "line": 1,
    "column": 9
  },
  "pos": 9
}
SyntaxError: Unterminated regular expression (1:9)

@tscpp what was your previous parser?

@KFlash
Copy link
Contributor

KFlash commented Nov 15, 2020

@3cp I will ask him. I know his language

@tscpp Kan du utarbeta lite mer om vad du menar? Alla "parsers" gör samma sak. Och i Meriyah är detta baserat på ECMA-standarden. Vad är skillnaden mellan Meriyah och den tidigare koden du använde? Kan det räcka att lägga till en extra "property"? Kommer det att hjälpa dig?

@tscpp
Copy link
Author

tscpp commented Nov 16, 2020

The projected used Acorn before. I switched to Meriyah because of the performance difference and the support for ES2020. We didn't log the syntax error location before. We logged the error to the whole line. I got suggested to log the syntax error meanwhile I was working with the pull request. We want support for end positions, but it was not available, so I created this issue.

If this is possible, my suggestion would be to add a property called range or start and end in ParserError before releasing a new master version. Also, could ParserError or an interface of ParserError be exported in the main module?


Adding the property start would be quite useless.

@KFlash
Copy link
Contributor

KFlash commented Nov 16, 2020

@3cp It shouldn't be too hard to add extra property? See this code. Loc already exist. Shouldn't affect the rest of the AST nodes.

@tscpp Sounds okay? I'm not sure what language you prefer? Är detta en okej lösning på problemet? För att lägga till det här?
Jag arbetade 11 år i grannlandet. Strax över gränsen talar också norska.

@tscpp
Copy link
Author

tscpp commented Nov 16, 2020

Jag föredrar engelska så att andra kan läsa mina kommentarer 😁


I'm making a pull request adding the property end to ParserError.

@tscpp
Copy link
Author

tscpp commented Nov 16, 2020

Should '[' + loc.start.line + ':' + loc.start.column + ']: ' be removed from the message. I'm finding this quite unseccesary. It should be added by the client.

@KFlash
Copy link
Contributor

KFlash commented Nov 16, 2020

I don't think it's an standard on this. All Parsers report this differently. Try to look what others do with AST explorer

@tscpp
Copy link
Author

tscpp commented Nov 16, 2020

Which properties in ParserState should I use?

@KFlash
Copy link
Contributor

KFlash commented Nov 16, 2020

See finishNode in common. TS file. Add start and if that solve the issue

@tscpp
Copy link
Author

tscpp commented Nov 16, 2020

I don't think it's an standard on this. All Parsers report this differently. Try to look what others do with AST explorer

Acorn uses SyntaxError which doesn't report the start position.

@tscpp
Copy link
Author

tscpp commented Nov 16, 2020

Because the variable node is not defined in every function in parser.ts, I need to define the variable node at 194 places in the code. I added the parameters context and node to the function report, and it seems like node is not defined anywhere. I also looked at the references of finishNode, and all of the expressions calling the function define node with properties type and body. I have body, but the problem is that I don't know type, I could define all node variables but it seems like a bit more work than I'm currently able to do. Do you still want to rest of the pull request?

@KFlash
Copy link
Contributor

KFlash commented Nov 16, 2020

cc @3cp Could you help out with this?

@3cp
Copy link
Member

3cp commented Nov 17, 2020

It's hard to follow without looking at the code. You can make a draft PR, doesn't need to be fully functional, but provide a base for us to discuss.

@tscpp
Copy link
Author

tscpp commented Nov 17, 2020

@3cp #157

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

No branches or pull requests

3 participants