JS: Add common interface between TypeExpr and JSDocTypeExpr#1257
Merged
xiemaisi merged 19 commits intogithub:masterfrom Apr 29, 2019
Merged
JS: Add common interface between TypeExpr and JSDocTypeExpr#1257xiemaisi merged 19 commits intogithub:masterfrom
xiemaisi merged 19 commits intogithub:masterfrom
Conversation
xiemaisi
reviewed
Apr 16, 2019
xiemaisi
left a comment
xiemaisi
left a comment
There was a problem hiding this comment.
Design LGTM; the potential breakage seems hard to avoid, but as long as we mention it in the release notes we should be fine.
| /** | ||
| * A type annotation, either in the form of a TypeScript type or a JSDoc comment. | ||
| */ | ||
| class TypeAnnotation extends @type_annotation { |
There was a problem hiding this comment.
I suppose we could make this extend Locatable?
Contributor
Author
There was a problem hiding this comment.
Done, although the location of the JSDoc annotations seems to be file://:0:0:0:0. I'll try and see if there's a simple fix for that.
There was a problem hiding this comment.
the location of the JSDoc annotations seems to be file://:0:0:0:0
That doesn't sound intentional. If you can fix it that would be great, but of course it's orthogonal to this PR.
Contributor
Author
There was a problem hiding this comment.
I think I fixed it. I'll just do it in the same PR.
xiemaisi
previously approved these changes
Apr 17, 2019
asger-semmle
added
the
depends on internal PR
xiemaisi
previously approved these changes
Apr 24, 2019
xiemaisi
previously approved these changes
Apr 26, 2019
Co-Authored-By: asger-semmle <42069257+asger-semmle@users.noreply.github.com>
xiemaisi
approved these changes
Apr 29, 2019
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Adds a class
TypeAnnotationwithTypeExprandJSDocTypeExpras subclasses.It has some methods for common tasks but currently doesn't a mirror hierarchy of the two class hierarchies. I'm not sure how much value we get out of modelling this hierarchy.
For now I'd like to feedback on whether this design is something we can all agree on.
The return type of
getTypeAnnotation()and friends have changed toTypeAnnotation, which is a breaking change since this is not anASTNode. I've copied over some of the predicates fromASTNodeto reduce the impact. Short of deprecating and renaming all of them, or changing JSDocTypeExpr to be an ASTNode, I think it's unavoidable but low impact.Still needs an upgrade script, and an evaluation is running in case changing the rootdef of
getEnclosingStmtand friends have catastrophic consequences.