Fix ambiguity between function signature and function declaration #126
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
resolritter
commented
Jan 7, 2021
resolritter
force-pushed
the
function_signature
branch
3 times, most recently
from
7656bdc to
dc392ff
Compare
Contributor
|
Looks good to me! @maxbrunsfeld, can we merge this? |
Contributor
There was a problem hiding this comment.
Thanks for looking into this @resolritter. Overall, I think your approach of having a special automatic semicolon token is a good one 👍 .
A few questions:
-
We should continue to distinguish between an actual semicolon (which should appear as a
;node in the tree) and an "automatic semicolon" (which is hidden). -
I think the Automatic Semicolon Insertion logic needs to check for a
:, because a type signature can appear on the next line, after the parameter list:function foo() : number; function foo() { return 1; }
-
We probably need to account for comments after the function signature:
function foo() // this is a signature foo(); function bar() // this is a definition { console.log("the function body"); }
-
There is some specific behavior in the above code (for matching normal automatic semicolons) that calls
mark_endto make sure that the location of the automatic semicolon is right at the start position. I think we need to do that here too.
Can this logic be more unified with the other automatic semicolon logic? Ideally, instead of checking iswalpha, we would specifically look for a {.
Contributor
Author
|
@maxbrunsfeld I find the comment one difficult to work around. My understanding is that, because i.e. because _call_signature: $ => seq(
field('type_parameters', optional($.type_parameters)),
field('parameters', $.formal_parameters),
+ optional($.comment)
field('return_type', optional(
choice($.type_annotation, $.asserts, $.type_predicate_annotation)
))
),it effectively cuts That explanation might not be precise as far as explaining the cause, but I hope the hurdle itself is understandable. I don't know how to work around it. |
Contributor
|
The existing It seems like the same logic could apply to |
resolritter
force-pushed
the
function_signature
branch
from
dc392ff to
08c4e7c
Compare
Contributor
Author
|
@maxbrunsfeld I've added your examples to the corpus. |
maxbrunsfeld
reviewed
Feb 3, 2021
common/define-grammar.js
Outdated
| ), | ||
| optional(";") | ||
| ) | ||
| )) |
Contributor
There was a problem hiding this comment.
I don't understand this change to the grammar. Why does _function_signature_semicolon come before the type signature?
Contributor
Author
There was a problem hiding this comment.
This one was tricky as I had to recreate my train-of-thought from yesterday...
_function_signature_semicolon does lexer->mark_end at the start and looks far ahead, thus the position in the tree does not matter significantly. Even more so in this case where there are two optional nodes following.
It appears before the type because it can match regardless of the type being there or not. If it were put in front, it would require that a type proceeded it, which is not necessary. Consider:
function bar();
No type annotation before ;, but still valid syntax nevertheless.
As to why the other more normal-looking rule is needed despite this, it's to catch the remaining cases where { might appear really far forward into the code after a sibling node has already started - which, conversely, means this one must have ended.
I'll take a look later if I can find an example in the wild which parses incorrectly with this approach.
resolritter
force-pushed
the
function_signature
branch
2 times, most recently
from
3d65a92 to
8057c92
Compare
Contributor
Author
|
Moving to draft while I try to find more examples in the wild which breaks this. Also will try to have it commented more when that's done. |
resolritter
force-pushed
the
function_signature
branch
from
8057c92 to
a8b7173
Compare
Contributor
Author
|
Split the node's logic between
It looks saner and more predictable now. Notably, it doesn't "look far into the future" anymore like the old approach did. Tests are passing for https://github.com/desktop/desktop but I was thinking of running it on another big code base such as https://github.com/microsoft/TypeScript/tree/master/src. I already tried running it on https://github.com/microsoft/TypeScript/tree/master/tests but it lead to lots of errors, some not necessarily related to this use-case; I don't think it's ready to run it on the official test suite just yet, but it would be good to, in the future. |
resolritter
force-pushed
the
function_signature
branch
from
a8b7173 to
789a0d0
Compare
Contributor
Author
|
I've tried to run it on the TypeScript source and it found a lot of errors. Too many to even go through them manually. Then I tried the vscode source - yet again, an astounding number of errors. From what I saw, some of those errors are related to conditional types, thus the situation could improve with #124. Anyways, I settled for the Redux code base which is smaller. It did help me find a missing case about For running the tests on TypeScript and vscode, since those repositories are big, I've had to improve the |
Contributor
This is great, thanks for improving that script, and taking the time to analyze some of the parse errors that you were seeing. I pushed up a commit that simplifies the |
resolritter
commented
Feb 5, 2021
| git pull --ff-only --depth 1 origin "$sha" | ||
| if [ "${folder:-}" ]; then | ||
| while IFS= read -r file; do | ||
| echo "${folder}" "$file" |
Contributor
Author
There was a problem hiding this comment.
This is a print debugging leftover. Not sure if you mind.
Contributor
There was a problem hiding this comment.
Oh yeah, I missed that. What is this while loop for?
Contributor
Author
There was a problem hiding this comment.
It's for cleaning unrelated folders before the **/*.ts glob. In the TypeScript repository there's the tests folder which has some broken files on purpose, so I thought I only wanted the src folder which is more certain to have only valid syntax.
This loop goes over the files in the directory and removes everything that's not $folder - in this case I was passing src as $folder since that's the only one I'm interested in.
Contributor
Author
|
Your change has resulted in a smaller for the same achievement, possibly faster as well due to less branching, so I think that's enough argument to get rid of the old approach no questions asked.
I only implemented it that other way because I found it easier to reason about expressing the rules as a separate node... But if it can be unified, that's good. Thanks for taking a look at this. |
Contributor
|
Thanks @resolritter. |
resolritter
commented
Feb 5, 2021
| while IFS= read -r file; do | ||
| echo "${folder}" "$file" | ||
| if [ "$file" != "$folder" ]; then | ||
| rm -r "$file" |
Contributor
Author
There was a problem hiding this comment.
This should be rm -rf instead of -r only because the ls -A will expose .git and other hidden files which might be write-protected. Otherwise, you'll get prompts like those:
> rm -r .git
rm: remove write-protected regular file '.git/objects/58/7ff082e0b98914788500eae5dd6a33f04883c9'? The intention is to remove anything unrelated to the $folder, so it makes sense to force removal with -f.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
closes #95