Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add typescript types to bin #538
Add typescript types to bin #538
Changes from 10 commits
2dd9c21
c10542e
97d47d2
1a2a771
04e522c
e204162
576c980
36a2f60
caecbb7
f895d44
792c1a3
620fdcf
4157552
349eb80
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The comments say this is an object. If this cannot be trusted then you should update the comment and pursue a different change. I can work with you on this.
To start off I see this code block:
offset
seems to only be true whenoptions.true
(within the compiler itself) is true. However this still indicates a shape.Maybe type it like so?
The inline comments there are for the benefit of explanation I would not leave them in.
On the other hand I see some internal code that suggests that it could be a string or a
File
or anything else. In which case I would suggestI wrote it this way for intellisense purposes. The type
{}
actually means "any type that isn't null or undefined" and then I addednull
andundefined
to allow both of those. This is better thanFile | string | unknown
because in intellisense it'd just collapse and showunknown
.Though I bet you probably just want some subset like objects/arrays/so on? For instance you probably don't want a function but I guess I don't know.
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.
Don't worry about being too pedantic. I indeed did ask for it. :)
Yeah, the comment is wrong.
grammarSource
is often a string containing the file name, but could also be an object that produces a useful string whenString(grammarSource)
is applied, e.g. something with atoString()
method. They also need to be testable for equality with===
. As such, numbers would work also, as long as they aren't NaN.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.
The GrammarLocation class in lib/grammar-location.js is a good example of why you might want an object as the grammarSource.
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.
And you're right that
GrammarLocation
has a few methods that the generated parser can use if they are there. They should be documented.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.
Yeah so my final type;
type Source = File | string | {} | null | undefined;
in the last comment may be close to what you want. It'll allow literally anything with some intellisense hints about common uses.But here's another try at it based upon your new constraints:
Notably this new version does NOT just allow anything but I left a comment about everything it omits.
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've got this:
That
unknown
is too broad, and I'd be open to just removing it in favor of the things we know work and are useful.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.
Alright, what things are known to work and are useful? I can help you figure that out.
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'm driving across Kansas today, currently charging the car in Maple Hill. Sorry in advance for spotty connectivity.
I think I'm probably the only one who ever uses anything other than a string, and the only other thing I ever use is GrammarLocation, which conforms to GrammarSourceObject. Let's just take the unknown out and let people file bugs when they want more. :)
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.
Is there any way I could avoid
any
here?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.
It's either that,
unknown
, or we put in another generic type, which will make the whole thing kind of a mess.It really can be almost anything, as long as it can be converted to a string with
String(source)
, and it matches thegrammarSource
that got passed in toparse
.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.
Since you asked for a more full review, yeah I'd suggest
unknown
orSource
here. You said it should matchgrammarSource
henceSource
(which I talked about typing in a different comment) if that even makes sense.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.
Pedantic; it's a soundness hole you practically have to try to cause in this case but it's an easy fix.
After this preface I'm going to jump into a deep explanation into intentional unsoundness of TypeScript. You can ignore all of what I'm about to say and simply try to address a possible unsoundness with this simple diff
By defining it this way you say that "trace" is a method. TypeScript handles this specially for classes and makes it "bivariant" - a term practically exclusive to TypeScript and makes it rather unsound. You can see this FAQ entry for their explanation of what bivariance is but in this specific case it lets you do unsound stuff like this:
See this playground if you want to see for yourself.
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.
Nod. That seems easy enough to fix.
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 can't mark this as resolved but it can be.
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.
Looking at the source this is generated in two places;
generateRuleFooter
which is called withstack.source()
which is always astring
and ingenerateRuleHeader
which is called withasts.indexOfRule(ast, rule.name)
which is always a number.There may be a reason I'm missing to keep it more generic but I believe
string | number
would suffice for the current runtime possibilities? Perhaps this will be changed in the future and is meant to be assumed to be arbitrary. In that case I would suggestunknown
.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 you might have been looking in the wrong place. Generate a grammar with
--trace
turned on to see the code that will run. Look forpeg$tracer.trace
calls. The result is anything that a rule could return from an action block. Example: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.
Aha! Yeah you're right, I see it in source code now too. My folly was checking for the string
"rule.match"
which only included the things I mentioned.Looks like
unknown
is the best type for the job unless action blocks can't return, say, functions or something.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 can totally imagine them returning a function. For instance, you might implement an XPath parser that way... (/me checks. No, I didn't do that in my XPath parser, but it would have been reasonable)
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 can't mark this as resolved but it can be.