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

Prevent infinite loop when scanning for line-breaks and there are comment-like strings #3069

Merged
merged 4 commits into from Aug 21, 2019

Conversation

lukastaegert
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Resolves #3067

Description

When fixing invalid generated code when simplifying return statement, a new bug was introduced because scanning for line-breaks would not always terminate properly when there were strings in the code that looked like comments. This is fixed here.

See #3067 (comment) for more info.

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #3069 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3069      +/-   ##
=========================================
+ Coverage   88.75%   88.8%   +0.05%     
=========================================
  Files         165     165              
  Lines        5737    5728       -9     
  Branches     1748    1743       -5     
=========================================
- Hits         5092    5087       -5     
  Misses        388     388              
+ Partials      257     253       -4
Impacted Files Coverage Δ
src/utils/renderHelpers.ts 100% <100%> (+5.55%) ⬆️

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 852e704...a0bf53e. Read the comment docs.

Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Changes look good from my end.

I don't know enough about this part of the code to know if it would be an issue, but would regular expressions get caught by the last commit?

@lukastaegert
Copy link
Member Author

Technically yes, just as the division operator or special strings would be caught. The point is that all of these functions are only called in scenarios where there is at most whitespace and comments in the searched range so they should be save. But of course this needs to be considered in all places where they are used. However I believe this "hidden" contract should be acceptable considering all of them are highly performance critical.

@shellscape
Copy link
Contributor

Shippppp it

@lukastaegert
Copy link
Member Author

Definitely! Will trigger it now...

@lukastaegert lukastaegert merged commit f6a6be0 into master Aug 21, 2019
@lukastaegert lukastaegert deleted the gh-3067-prevent-pseudo-comment-infinite-loop branch August 21, 2019 15:56
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.

Rollup v1.19.4 freezes when trying to bundle our frontend app
2 participants