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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary wraps for non built-in functions #1126

Merged
merged 3 commits into from
Feb 16, 2021
Merged

Remove unnecessary wraps for non built-in functions #1126

merged 3 commits into from
Feb 16, 2021

Conversation

RageKnify
Copy link
Contributor

Lint was introduced with Rust 1.50
Ignoring the lint for built-in functions because they have a fixed
signature

It changes the following:

  • Remove Result from return types where possible
    -Add allow clippy directives for built-in functions that since those must return Result<Value>

@RageKnify
Refactor: Remove unnecessary_wraps for non built-in functions
f4b82fa 
Lint was introduced with Rust 1.50
Ignoring the lint for built-in functions because they have a fixed
signature
@RageKnify RageKnify added the builtins PRs and Issues related to builtins/intrinsics label Feb 12, 2021
@github-actions
Copy link

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 24,986 24,986 0
Ignored 15,587 15,587 0
Failed 37,924 37,924 0
Panics 16 16 0
Conformance 31.83 31.83 0.00%

@codecov
Copy link

codecov bot commented Feb 12, 2021 •
edited
Loading

Codecov Report

Merging #1126 (a31cc43) into master (9e0cab5) will decrease coverage by 0.14%.
The diff coverage is 46.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1126      +/-   ##
==========================================
- Coverage   58.53%   58.38%   -0.15%     
==========================================
  Files         176      176              
  Lines       12547    12579      +32     
==========================================
  Hits         7344     7344              
- Misses       5203     5235      +32
Impacted Files Coverage Δ
boa/src/builtins/array/array_iterator.rs 76.59% <0.00%> (-2.13%) ⬇️
boa/src/builtins/console/mod.rs 22.45% <ø> (ø)
boa/src/builtins/math/mod.rs 51.87% <ø> (ø)
boa/src/builtins/number/mod.rs 66.31% <ø> (ø)
boa_tester/src/exec.rs 0.00% <0.00%> (ø)
boa/src/syntax/parser/expression/assignment/mod.rs 46.73% <12.50%> (-3.27%) ⬇️
boa/src/builtins/string/mod.rs 62.91% <16.66%> (ø)
...arser/expression/primary/object_initializer/mod.rs 64.35% <25.00%> (-1.31%) ⬇️
boa/src/builtins/map/map_iterator.rs 67.85% <50.00%> (-1.79%) ⬇️
boa/src/builtins/map/mod.rs 69.14% <50.00%> (-1.95%) ⬇️
... and 10 more

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 9e0cab5...13cc560. Read the comment docs.

@github-actions
Copy link

Benchmark for a31cc43

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 356.2±11.49ns 345.6±15.33ns +3.07%
Arithmetic operations (Full) 231.1±1.65µs 228.2±4.83µs +1.27%
Array access (Execution) 5.2±0.34µs 5.9±0.24µs -11.86%
Array access (Full) 256.5±0.48µs 238.9±14.48µs +7.37%
Array creation (Execution) 3.0±0.10ms 2.9±0.14ms +3.45%
Array creation (Full) 3.3±0.00ms 3.2±0.11ms +3.13%
Array pop (Execution) 947.5±29.93µs 877.8±44.83µs +7.94%
Array pop (Full) 1421.1±3.43µs 1347.5±61.28µs +5.46%
Boolean Object Access (Execution) 5.2±0.04µs 5.2±0.18µs 0.00%
Boolean Object Access (Full) 251.5±2.48µs 250.7±2.52µs +0.32%
Clean js (Execution) 685.3±9.44µs 603.4±27.88µs +13.57%
Clean js (Full) 980.3±12.14µs 984.9±12.60µs -0.47%
Clean js (Parser) 43.5±0.04µs 42.9±0.95µs +1.40%
Create Realm 403.1±25.73ns 463.9±8.34ns -13.11%
Dynamic Object Property Access (Execution) 5.1±0.01µs 4.6±0.31µs +10.87%
Dynamic Object Property Access (Full) 251.6±2.14µs 242.0±11.79µs +3.97%
Expression (Parser) 7.0±0.02µs 7.0±0.09µs 0.00%
Fibonacci (Execution) 757.5±53.15µs 830.3±15.57µs -8.77%
Fibonacci (Full) 1139.3±7.45µs 1100.7±32.82µs +3.51%
For loop (Execution) 20.6±1.32µs 22.6±0.54µs -8.85%
For loop (Full) 273.2±3.80µs 264.5±11.32µs +3.29%
For loop (Parser) 20.5±0.27µs 20.8±0.14µs -1.44%
Goal Symbols (Parser) 14.5±0.02µs 14.5±0.14µs 0.00%
Hello World (Parser) 3.9±0.03µs 3.9±0.01µs 0.00%
Long file (Parser) 798.0±2.13ns 791.9±1.29ns +0.77%
Mini js (Execution) 615.1±4.76µs 551.1±32.47µs +11.61%
Mini js (Full) 886.7±10.88µs 894.3±14.66µs -0.85%
Mini js (Parser) 38.4±0.06µs 38.2±0.51µs +0.52%
Number Object Access (Execution) 4.0±0.08µs 3.5±0.27µs +14.29%
Number Object Access (Full) 241.1±8.88µs 240.4±5.09µs +0.29%
Object Creation (Execution) 4.3±0.05µs 4.1±0.22µs +4.88%
Object Creation (Full) 247.8±0.43µs 240.2±10.57µs +3.16%
RegExp (Execution) 11.0±0.04µs 10.1±0.59µs +8.91%
RegExp (Full) 254.5±0.36µs 252.1±5.45µs +0.95%
RegExp Literal (Execution) 10.9±0.15µs 9.9±0.58µs +10.10%
RegExp Literal (Full) 259.2±1.36µs 245.7±11.57µs +5.49%
RegExp Literal Creation (Execution) 9.4±0.25µs 9.3±0.59µs +1.08%
RegExp Literal Creation (Full) 258.7±41.09µs 249.3±5.36µs +3.77%
Static Object Property Access (Execution) 4.5±0.15µs 4.2±0.43µs +7.14%
Static Object Property Access (Full) 250.3±0.41µs 245.3±5.11µs +2.04%
String Object Access (Execution) 6.8±0.29µs 6.6±0.44µs +3.03%
String Object Access (Full) 255.7±1.48µs 253.9±5.37µs +0.71%
String comparison (Execution) 6.7±0.06µs 5.8±0.45µs +15.52%
String comparison (Full) 252.6±1.36µs 243.9±9.80µs +3.57%
String concatenation (Execution) 5.2±0.02µs 4.5±0.33µs +15.56%
String concatenation (Full) 247.2±1.65µs 224.6±14.07µs +10.06%
String copy (Execution) 4.0±0.26µs 3.6±0.27µs +11.11%
String copy (Full) 241.5±0.92µs 228.4±13.27µs +5.74%
Symbols (Execution) 3.0±0.19µs 3.3±0.02µs -9.09%
Symbols (Full) 231.2±7.66µs 212.2±11.08µs +8.95%

HalidOdat
HalidOdat reviewed Feb 16, 2021
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good!

But I think it would be better if we allow clippy::unnecessary_wraps lint in the crate scope, since we have to annotate so many builtin js functions manually.

@RageKnify
Copy link
Contributor Author

Looks good!

But I think it would be better if we allow clippy::unnecessary_wraps lint in the crate scope, since we have to annotate so many builtin js functions manually.

I feel like this is a useful lint, it helps keep functions clean and we do we have a lot of functions.
Maybe we could allow it specifically in the buiiltins module since that's where most of them are? What do you think of that?

@HalidOdat
Copy link
Member

HalidOdat commented Feb 16, 2021 •
edited
Loading

Maybe we could allow it specifically in the buiiltins module since that's where most of them are? What do you think of that?

Yes. this seems like the best approach, instead of crate level :)

RageKnify added a commit that referenced this pull request Feb 16, 2021
@RageKnify
Refactor: Allow clippy::unnecessary_wraps at builtins module
ce1c9f7 
Discussed in #1126 with @HaldiOdat
@RageKnify RageKnify force-pushed the refactor/clippy-1.50-lints branch from ce1c9f7 to 13cc560 Compare February 16, 2021 18:59
@RageKnify RageKnify mentioned this pull request Feb 16, 2021
Merged
@github-actions
Copy link

Benchmark for 3b0e932

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 339.0±14.71ns 348.9±12.89ns -2.84%
Arithmetic operations (Full) 215.4±9.59µs 226.6±5.87µs -4.94%
Array access (Execution) 5.9±0.22µs 5.9±0.22µs 0.00%
Array access (Full) 253.1±2.44µs 236.0±9.56µs +7.25%
Array creation (Execution) 2.9±0.09ms 2.8±0.11ms +3.57%
Array creation (Full) 3.1±0.14ms 3.1±0.11ms 0.00%
Array pop (Execution) 940.7±20.83µs 913.4±32.49µs +2.99%
Array pop (Full) 1330.5±50.16µs 1297.8±69.02µs +2.52%
Boolean Object Access (Execution) 5.1±0.16µs 5.0±0.23µs +2.00%
Boolean Object Access (Full) 233.9±8.39µs 247.0±8.33µs -5.30%
Clean js (Execution) 644.4±26.24µs 646.5±22.75µs -0.32%
Clean js (Full) 918.1±31.19µs 954.3±19.36µs -3.79%
Clean js (Parser) 40.7±1.71µs 40.0±1.98µs +1.75%
Create Realm 434.1±19.90ns 427.0±20.30ns +1.66%
Dynamic Object Property Access (Execution) 5.0±0.05µs 4.7±0.22µs +6.38%
Dynamic Object Property Access (Full) 240.2±8.09µs 229.0±9.97µs +4.89%
Expression (Parser) 6.8±0.66µs 6.7±0.21µs +1.49%
Fibonacci (Execution) 834.4±20.27µs 801.8±36.10µs +4.07%
Fibonacci (Full) 1132.7±18.79µs 1046.6±44.22µs +8.23%
For loop (Execution) 21.3±1.03µs 21.7±0.96µs -1.84%
For loop (Full) 264.3±12.96µs 259.2±13.44µs +1.97%
For loop (Parser) 19.5±0.80µs 19.8±0.56µs -1.52%
Goal Symbols (Parser) 13.8±0.51µs 13.7±0.59µs +0.73%
Hello World (Parser) 3.6±0.21µs 3.7±0.16µs -2.70%
Long file (Parser) 744.7±39.01ns 773.7±23.30ns -3.75%
Mini js (Execution) 583.3±21.27µs 568.4±28.34µs +2.62%
Mini js (Full) 827.0±31.35µs 858.0±26.00µs -3.61%
Mini js (Parser) 36.5±1.40µs 36.9±1.18µs -1.08%
Number Object Access (Execution) 3.8±0.16µs 3.9±0.14µs -2.56%
Number Object Access (Full) 230.8±9.12µs 234.4±8.14µs -1.54%
Object Creation (Execution) 4.2±0.08µs 4.0±0.17µs +5.00%
Object Creation (Full) 230.0±9.58µs 224.0±10.74µs +2.68%
RegExp (Execution) 10.5±0.41µs 10.7±0.37µs -1.87%
RegExp (Full) 238.8±12.13µs 252.4±5.27µs -5.39%
RegExp Literal (Execution) 10.7±0.28µs 10.9±0.36µs -1.83%
RegExp Literal (Full) 245.9±9.70µs 242.4±7.53µs +1.44%
RegExp Literal Creation (Execution) 8.9±0.40µs 9.2±0.41µs -3.26%
RegExp Literal Creation (Full) 240.8±6.34µs 232.6±9.20µs +3.53%
Static Object Property Access (Execution) 4.7±0.22µs 4.3±0.17µs +9.30%
Static Object Property Access (Full) 234.2±8.92µs 227.6±11.03µs +2.90%
String Object Access (Execution) 6.7±0.30µs 6.8±0.21µs -1.47%
String Object Access (Full) 240.0±12.49µs 243.1±10.08µs -1.28%
String comparison (Execution) 6.3±0.23µs 6.3±0.22µs 0.00%
String comparison (Full) 246.0±5.53µs 246.2±5.57µs -0.08%
String concatenation (Execution) 4.9±0.19µs 5.1±0.21µs -3.92%
String concatenation (Full) 230.9±10.83µs 237.4±6.67µs -2.74%
String copy (Execution) 3.8±0.15µs 3.7±0.16µs +2.70%
String copy (Full) 224.4±8.98µs 227.9±7.76µs -1.54%
Symbols (Execution) 3.2±0.14µs 3.1±0.16µs +3.23%
Symbols (Full) 220.3±8.61µs 215.1±10.56µs +2.42%

@jasonwilliams
Copy link
Member

Looks good to me, agree with the above, I like this lint and if we really need to avoid it we should scope that down as much as possible

HalidOdat
HalidOdat approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good to me! :)

@HalidOdat HalidOdat merged commit 72313a8 into master Feb 16, 2021
@HalidOdat HalidOdat deleted the refactor/clippy-1.50-lints branch February 16, 2021 19:56
@RageKnify RageKnify mentioned this pull request Mar 10, 2021
Merged
Razican added a commit that referenced this pull request May 22, 2021
@Razican
Remove unnecessary wraps for non built-in functions (#1126)
133eb04
Razican added a commit that referenced this pull request May 22, 2021
@Razican
Remove unnecessary wraps for non built-in functions (#1126)
5381ac9
@RageKnify RageKnify added this to the v0.12.0 milestone May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HalidOdat HalidOdat HalidOdat approved these changes

@jasonwilliams jasonwilliams jasonwilliams approved these changes

@Razican Razican Awaiting requested review from Razican

@tofpie tofpie Awaiting requested review from tofpie

Assignees
No one assigned
Labels
builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@RageKnify @HalidOdat @jasonwilliams