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

location annotation #43

Closed
jeremy-coleman opened this issue Sep 6, 2019 · 13 comments
Closed

location annotation #43

jeremy-coleman opened this issue Sep 6, 2019 · 13 comments

Comments

@jeremy-coleman
Copy link

acorn and typescript output the line annotations as loc.line and loc.column, but meriyah just adds it as line and column. Even though this is a relatively simple thing to fix, in practice it's going to create a lot of bugs when trying to replace acorn. (I rewrote browserify to use meriyah yesterday, and this came up)

@KFlash
Copy link
Contributor

KFlash commented Sep 6, 2019

Hi. Out of the box Acorn outputs both line and column node on the AST node. This is off by default in Meriyah, but you can use the range option to enable it.
If you enable the loc option you will get a loc object equal to Acorn.

"loc": {
    "start": {line: Number, column: Number}
    "end": {line: Number, column: Number}
  },

The only differences between Acorn and Meriyah when it comes to location tracking is the ranges option. It was a design choice due to performance. Acorn output this on the AST node
if ranges is set "range": [Number, Number], but Meriyah set the start and end properties.

So the outputed AST with location tracking on is the same for both Meriyah and Acorn. You can test it in AST Explorer, but if you feel anything should be different, you are welcome to send a PR. I'm in the middle of stress testing my Typescript parser, so little busy until next week.

@jeremy-coleman
Copy link
Author

jeremy-coleman commented Sep 6, 2019

i had to change error handles like this

        this.message = err.message.replace(/\s+\(\d+:\d+\)$/, '');
        this.line = err.line;
        this.column = err.column + 1;

        //this.line = err.loc.line; <----Original 
        //this.column = err.loc.column + 1; <----Original
        
        this.annotated = '\n'
            + (file || '(anonymous file)')
            + ':' + this.line
            + '\n'
            + src.split('\n')[this.line - 1]
            + '\n'
            + Array(this.column).join(' ') + '^'
            + '\n'
            + 'ParseError: ' + this.message
        ;
    }

Without the change , it throws with err.loc.line is undefined. Changing to err.line fixes it. (This is with line: true)

@KFlash
Copy link
Contributor

KFlash commented Sep 6, 2019

Ah! You was talking about the error reporting :) Yes. That's different because Acorn uses a loc object and createa a new SyntaxError, but I'm extending the SyntaxError class and passing the line, column values as func args. err.loc.column also performs worse than err.column

@jeremy-coleman
Copy link
Author

jeremy-coleman commented Sep 6, 2019

putting them under .loc would make it possible to drop in as an acorn replacement for all the existing tools, i think the benefits there probably outweigh the perf cost?

this is the equivalent ParserError extending SyntaxError (for use in browserify) with required changes

class ParseError extends SyntaxError {
    line: any
    column: any
    annotated: string
    constructor(err, src, file){
        super(...arguments)

        this.message = err.message.replace(/\s+\(\d+:\d+\)$/, '');

        //console.log(err)
        this.line = err.line;
        this.column = err.column + 1;

        //this.line = err.loc.line;
        //this.column = err.loc.column + 1;
        
        this.annotated = '\n'
            + (file || '(anonymous file)')
            + ':' + this.line
            + '\n'
            + src.split('\n')[this.line - 1]
            + '\n'
            + Array(this.column).join(' ') + '^'
            + '\n'
            + 'ParseError: ' + this.message
        ;
    }

    toString = () => this.annotated;
    inspect = () => this.annotated;
}

I get that it's a very small detail, but it does prevent meriyah from being able to easily replace acorn :(

@KFlash
Copy link
Contributor

KFlash commented Sep 6, 2019

I get the point, and yes I'm agree that this could be changed. Is this something you can make a PR for? I think maybe it's an idea just to add a new option - acorn and store a boolean value on parser state and check if the value is set when throwing an error.

@jeremy-coleman
Copy link
Author

jeremy-coleman commented Sep 6, 2019

something like this?

export class ParseError extends SyntaxError {
 public loc: {
 line: ParseError["line"],
 column: ParseError["column"]
}
  public index: number;
  public line: number;
  public column: number;
  public description: string;
  /*eslint-disable*/
  constructor(startindex: number, line: number, column: number, type: Errors, ...params: string[]) {
    const message =
      '[' + line + ':' + column + ']: ' + errorMessages[type].replace(/%(\d+)/g, (_: string, i: number) => params[i]);
    super(`${message}`);
    this.index = startindex;
    this.line = line;
    this.column = column;

//this is probably better than my first comment
//if loc already is there, just direct assign.
        if(this.loc){
            this.loc.line = line
            this.loc.column = column
        }
//if not, init with proper object shape 
        if(!this.loc) this.loc = {
            line: line || 1,
            column: column || 1
        }

    this.description = message;
  }
}

ahh no this doesn't fix the problem, would need to extend this in all the consuming libs , which doesnt fix anything lol

@KFlash KFlash closed this as completed in a474cd7 Sep 6, 2019
@KFlash
Copy link
Contributor

KFlash commented Sep 6, 2019

Did some changes and pushed to NPM. See if that helps you :)

@jeremy-coleman
Copy link
Author

cool , thank you! I'll let you know in a just a sec

@jeremy-coleman
Copy link
Author

jeremy-coleman commented Sep 6, 2019

Perfect, thank you!
before
image

after
image

(syntax error caused by importing css without a transform)

@KFlash
Copy link
Contributor

KFlash commented Sep 6, 2019

Good to know it solved :) If you face other issues don't hesitate to open an issue. I always try to respond and solve the issues within 1 hour from the moment it's reported.

@jeremy-coleman
Copy link
Author

jeremy-coleman commented Sep 6, 2019

Will do. I'm almost finished with this refactor of browserify, i'll put it in a repo and send you a link when its done , maybe some good marketing for meriyah? ;p

you might find this marginally helpful for your ts integration wtih fusebox (it's just a single file typed version of tsify), i love the format of the error output
https://github.com/jeremy-coleman/tsify/blob/master/tsify.ts

@KFlash
Copy link
Contributor

KFlash commented Sep 6, 2019

I guess so :) Do you need to do a massive refactoring to get it working with browserify?

I looked at the file and I can try to "copy" that error output for both Meriyah and Buntis soon after I have pushed to public a prototype of the TS parser.

@jeremy-coleman
Copy link
Author

jeremy-coleman commented Sep 7, 2019

@KFlash ok-ish for now, got some follow up to do. everything is in tools, the src folder is a demo app.
https://github.com/jeremy-coleman/meriyahify-demo

it didnt take much , i just directly replaced all calls to acorn.parse with meriyah's parse and had to enable loc and ranges in ./tools/packflat ~line 250.

I inlined almost all the dependencies and removing stuff. I also completely removed the original browser-pack, and replaced with packflat (https://github.com/goto-bus-stop/browser-pack-flat) , which is typically used in production builds by default in the tinyify plugin. As you can probably see though, there's tons of redundant utility libs, i'd like to delete most of them.

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

2 participants