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

[typescript] Probable bug in parsing #1713

Closed
msorens opened this issue Sep 21, 2020 · 12 comments
Closed

[typescript] Probable bug in parsing #1713

msorens opened this issue Sep 21, 2020 · 12 comments
Assignees
Labels
bug Something isn't working fixed lang:typescript user:external requested by someone outside of r2c

Comments

@msorens
Copy link

msorens commented Sep 21, 2020

Describe the bug

Running semgrep on my large-ish code base yielded these remarkably few probable parsing problems--nice job, folks!

These files compile cleanly and are currently running in production so, as far as I know, they should be parsable.

warn: parse error
  --> data-bags.requests.ts:1
1 | import { Injectable } from '@angular/core';
  | 
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse data-bags.requests.ts as ts

warn: parse error
  --> cookbook-details.component.ts:1
1 | import { Component, OnInit, OnDestroy } from '@angular/core';
  | 
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse cookbook-details.component.ts as ts

warn: parse error
  --> entities.ts:1
1 | import {
  | 
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse entities.ts as ts

To Reproduce

My codebase is open source so you can go right to, well, the source: https://github.com/chef/automate/tree/master/components/automate-ui

Expected behavior
No parsing errors.

Environment
Version 0.24.0

@mjambon
Copy link
Member

mjambon commented Sep 22, 2020

Thanks for the report, @msorens. I'll look into this.

@nbrahms nbrahms added the user:external requested by someone outside of r2c label Sep 29, 2020
aryx added a commit that referenced this issue Oct 1, 2020
There is already some code to automatically intercept Parse_info exns
to display their location, so better to use a consistent scheme
across pfff and tree-sitter parsers.

test plan:
```
(semgrep) pad@yrax:~/semgrep/tests/OTHER/parsing_errors$ semgrep -l ts -e 'FOO' .
warn: parse error
  --> err.ts:2
2 |          return 1+
  |          ^^^^^^^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse err.ts as ts

Warnings exist. Run with `--strict` to turn warnings into errors.
```

better than the previous error that was always reported on the first line.
This also improves error location for the errors reported in
#1713
@aryx
Copy link
Collaborator

aryx commented Oct 1, 2020

With the better error reporting I now get those parse error locations:

warn: parse error
  --> entities.ts:20
20 |   readonly [id: string]: T;
   |             ^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse entities.ts as ts

warn: parse error
  --> data-bags.requests.ts:20
20 |   : Observable<DataBagDetailsSuccessPayload> {
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse data-bags.requests.ts as ts

Warnings exist. Run with `--strict` to turn warnings into errors.

@aryx
Copy link
Collaborator

aryx commented Oct 1, 2020

This can help in the futur to provide more information on the kind of constructs we do not parse well, which also makes it
easier to create simpler test file showing the issue (minimal test case).

@msorens
Copy link
Author

msorens commented Oct 1, 2020

Some additional feedback: I just upgraded from 0.24 to 0.26 and I am seeing errors on the same two files as you show @aryx , but they are different errors !?!

warn: parse error
  --> data-bags.requests.ts:1
1 | import { Injectable } from '@angular/core';
  | 
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse data-bags.requests.ts as ts

warn: parse error
  --> entities.ts:1
1 | import {
  | 
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse entities.ts as ts

@aryx
Copy link
Collaborator

aryx commented Oct 1, 2020

yes the latest code for better error location is not yet in the release, it will be in 0.27

aryx added a commit that referenced this issue Oct 2, 2020
There is already some code to automatically intercept Parse_info exns
to display their location, so better to use a consistent scheme
across pfff and tree-sitter parsers.

test plan:
```
(semgrep) pad@yrax:~/semgrep/tests/OTHER/parsing_errors$ semgrep -l ts -e 'FOO' .
warn: parse error
  --> err.ts:2
2 |          return 1+
  |          ^^^^^^^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse err.ts as ts

Warnings exist. Run with `--strict` to turn warnings into errors.
```

better than the previous error that was always reported on the first line.
This also improves error location for the errors reported in
#1713
@msorens
Copy link
Author

msorens commented Oct 9, 2020

Just to be explicit, with 0.27.0 I am now seeing the revised error messages @aryx indicated. However, those files are all valid code, so those errors should not be present.

@mjambon
Copy link
Member

mjambon commented Oct 15, 2020

@msorens a number of fixes are coming for typescript but still need integration work. They will become available most likely in the semgrep release two weeks from now. This includes:

@mjambon mjambon closed this as completed Oct 15, 2020
@aryx aryx reopened this Oct 16, 2020
@aryx
Copy link
Collaborator

aryx commented Oct 16, 2020

Let's keep this open until we double check that the new release really fix the issue.

@aryx aryx added the fixed label Oct 16, 2020
@aryx
Copy link
Collaborator

aryx commented Oct 16, 2020

Unless this is already working with the develop version @mjambon ?

@dlukeomalley
Copy link
Member

dlukeomalley commented Oct 20, 2020

@aryx @mjambon This appears to still not parse. I ran a subset of the rules on one of the problematic files via Semgrep.dev:

❌ 0.27.0: https://semgrep.dev/s/GRBn/?version=0.27.0
❌ develop: https://semgrep.dev/s/GRBn/?version=develop

@aryx
Copy link
Collaborator

aryx commented Nov 19, 2020

Can we close this now?

@mjambon
Copy link
Member

mjambon commented Nov 24, 2020

This is now fixed:

warn: parse error
  --> entities.ts:20
20 |   readonly [id: string]: T;
   |             ^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse entities.ts as ts

This is fixed too:

warn: parse error
  --> data-bags.requests.ts:20
20 |   : Observable<DataBagDetailsSuccessPayload> {
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse data-bags.requests.ts as ts

It's all good now, already released.

@mjambon mjambon closed this as completed Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed lang:typescript user:external requested by someone outside of r2c
Development

No branches or pull requests

6 participants