-
Notifications
You must be signed in to change notification settings - Fork 14
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
track line and column #59
Conversation
0149581
to
393cd89
Compare
@rbuckton I addressed your comments; do you want to take another look? |
Out of curiosity, what does ecmarkdown consider to be a "line terminator"? Not all languages/editors agree ([1], [2]), so we should make sure ecmarkup, ecmarkdown, and grammarkdown agree. [1] https://twitter.com/SeaRyanC/status/1253037372263952387 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor suggestions, but you're welcome to ignore them.
src/parser.ts
Outdated
@@ -365,31 +370,32 @@ export class Parser { | |||
|
|||
pushPos() { | |||
if (this._posStack) { | |||
this._posStack.push(this.getPos()); | |||
this._posStack.push(this.getPos() as Position); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: TS has a postfix-!
syntax as a simple cast to remove null
/undefined
, but just as with the as
cast, both are type-level only with no runtime enforcement.
This is also possibly unsound if somehow you end up with a defined this._posStack
but an undefined node.location
. Perhaps a better approach would be ensure the value is defined:
this._posStack.push(this.getPos() as Position); | |
const pos = this.getPos(); | |
if (pos) { | |
this._posStack.push(pos); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you could change the body in this way (if you are using TS 3.7 or later):
pushPos() {
const pos = this.getPos();
if (pos) this._posStack?.push(pos);
}
src/parser.ts
Outdated
} | ||
} | ||
|
||
popPos() { | ||
return this._posStack ? this._posStack.pop() : -1; | ||
return this._posStack ? this._posStack.pop() : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this._posStack ? this._posStack.pop() : undefined; | |
return this._posStack?.pop(); |
src/parser.ts
Outdated
return this._posStack && tok.location ? tok.location.pos : -1; | ||
// TODO rename to getStart ? | ||
getPos(node: Node | Token = this._t.peek()) { | ||
return this._posStack && node.location ? node.location.start : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this._posStack && node.location ? node.location.start : undefined; | |
return this._posStack && node.location?.start; |
src/parser.ts
Outdated
} | ||
|
||
getEnd(node: Node | Token) { | ||
return this._posStack && node.location ? node.location.end : -1; | ||
return this._posStack && node.location ? node.location.end : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this._posStack && node.location ? node.location.end : undefined; | |
return this._posStack && node.location?.end; |
let actualStart: Position = start ?? (this.popPos() as Position); | ||
let actualEnd: Position = | ||
end ?? | ||
(this._t.previous === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could also be:
let actualEnd: Position = end ?? { line: 1, column: 1, offset: 0, ...this._t.previous?.location?.end };
src/tokenizer.ts
Outdated
@@ -425,12 +429,20 @@ export class Tokenizer { | |||
} | |||
} | |||
|
|||
enqueueLookahead(tok: Token, pos: number) { | |||
getLocation() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLocation() { | |
getLocation(): Position { |
Useful if you decide to rename or add a new member.
src/parser.ts
Outdated
@@ -306,7 +310,8 @@ export class Parser { | |||
this._t.next(); | |||
} | |||
|
|||
return this.finish({ name: 'text', contents }); | |||
let endLoc = !this._posStack || lastRealTok === null ? undefined : lastRealTok.location!.end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let endLoc = !this._posStack || lastRealTok === null ? undefined : lastRealTok.location!.end; | |
let endLoc = this._posStack && lastRealTok?.location?.end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastRealTok?.location?.end
The !
is fine after location
because it really is always defined if it's reached - this._posStack
being truthy implies every token has a location
property.
... Well, that's what I thought, but using lastRealTok?.location!.end
breaks tests. Which is surprising to me; I had thought the !
was just a type assertion, but it appears to be changing the semantics of the generated code. I guess TS is parsing it as the end of the "chain"? That's really surprising to me.
... Oh, I see this has already been discovered and fixed upstream. I guess I'll just // @ts-ignore
this line until that's released.
For parsing or emit? Only supporting |
For both. I mostly would like to maintain ecmarkdown as a tool specifically for processing parts of the ECMAScript specification. In that context, the goal is not to accept all reasonable inputs, but rather to accept only a very constrained set of inputs and reject all others, so that there are as few ways of writing the spec as possible. Concretely, if a contributor to ECMA-262 mixed CRLF line endings into an algorithm, it would be rejected by me in my capacitor as editor, which means it should ideally be rejected by the tooling so that the contributor can find that out for themselves. (That said, we should certainly at least give better errors than the generic parse error or malformed output you'd get now.) Is there a particular reason to support inputs which would be rejected as parts of ECMA-262?
Well, I'm also planning on changing that, since it seems like overkill for what this tool is actually used for. |
cc @rbuckton; this changes location tracking to include line and column numbers, not just offsets.