tdewolff / parse

Go parsers for web formats
MIT License
413 stars 65 forks source link

JS: Fixed panics in AST walker #74

Closed karelorigin closed 3 years ago

karelorigin commented 3 years ago

When we removed the double nil check in https://github.com/tdewolff/parse/commit/7116c70a8e8b5751f85475f8147cd5747c0f5fe1, we accidentally reintroduced panics into the AST walker.

This happened because the following line can evaluate to false when the interface, but not the underlying data structure, is nil: https://github.com/karelorigin/parse/blob/803d5a5faa635f7ca194fd311d05ce600e052ee4/js/walk.go#L13

You can find some examples here: https://yourbasic.org/golang/gotcha-why-nil-error-not-equal-nil/

I also added the relevant unit tests to prevent it from popping up again in the future.

codecov-commenter commented 3 years ago

Codecov Report

Merging #74 (803d5a5) into master (0ecee4d) will increase coverage by 1.79%. The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   93.77%   95.57%   +1.79%     
==========================================
  Files          29       29              
  Lines        5385     5396      +11     
==========================================
+ Hits         5050     5157     +107     
+ Misses        295      174     -121     
- Partials       40       65      +25     
Impacted Files Coverage Δ
js/ast.go 80.47% <ø> (+0.11%) :arrow_up:
js/walk.go 72.02% <8.33%> (+54.34%) :arrow_up:

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 0ecee4d...803d5a5. Read the comment docs.

karelorigin commented 3 years ago

Hey, kind of forgot about this PR, sorry! In my opinion, it's better to perform is-nil checks on any pointer. While I understand that some nodes will never be nil when coming from the parser itself, we should also take 'artificial' nodes into account. For example:

// contains nil nodes
n := js.ForInStmt{}
Walk(&walker{}, n)

Also, it's more convenient for unit tests as you can see in walk_test.go, all I had to do was add the struct to the list.

Let me know what you think :)

tdewolff commented 3 years ago

Good point, I've merged the PR, thanks for the effort!