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

Allow super.method() calls in constructor #324

Merged
merged 2 commits into from
Oct 28, 2018

Conversation

arv
Copy link
Contributor

@arv arv commented Oct 26, 2018

The old code assumed that super always followed by a ( which is not
the case.

Also adds tests for invalid (at runtime) code with multiple super()
and super.m() / super.x / super.x = 1 before super().

Fixes #323

The old code assumed that `super` always followed by a `(` which is not
the case.

Also adds tests for invalid (at runtime) code with multiple `super()`
and `super.m()` / `super.x` / `super.x = 1` before `super()`.

Fixes alangpierce#323
@arv
Copy link
Contributor Author

arv commented Oct 26, 2018

@alangpierce PTAL

I added support and tests for multiple super() and super.a access before super() even though all those cases would lead to runtime errors. Is it worth dealing with this or can we just assume that we do not get this kind of invalid input code?

@codecov-io
Copy link

codecov-io commented Oct 26, 2018

Codecov Report

Merging #324 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   76.61%   76.62%   +<.01%     
==========================================
  Files          41       41              
  Lines        5456     5458       +2     
  Branches     1279     1279              
==========================================
+ Hits         4180     4182       +2     
  Misses        900      900              
  Partials      376      376
Impacted Files Coverage Δ
src/util/getClassInfo.ts 85% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cbe6fc...93c27c1. Read the comment docs.

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Is it worth dealing with this or can we just assume that we do not get this kind of invalid input code?

I think it depends on how it affects the implementation. If invalid input makes the implementation harder, it's fine to call it out of scope and say the behavior is undefined. In this case, it's just the natural consequence of the straightforward approach that you took, so it's fine for Sucrase to handle it, and makes sense to add some tests as you did. Sucrase is intentionally not trying to check your program for errors (linters and typecheckers should be doing that), and a lot of parsing validation was intentionally removed for performance reasons, so there isn't any obligation to have Sucrase fail on the invalid cases that you tested for.

@alangpierce alangpierce merged commit 59159cb into alangpierce:master Oct 28, 2018
@arv arv deleted the super-methods branch October 29, 2018 17:18
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

Successfully merging this pull request may close these issues.

3 participants