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

Improves DCA warnings after divergent expressions. #5726

Merged
merged 14 commits into from
Apr 11, 2024
Merged

Improves DCA warnings after divergent expressions. #5726

merged 14 commits into from
Apr 11, 2024

Conversation

esdrubal
Copy link
Contributor

@esdrubal esdrubal commented Mar 13, 2024 •
edited
Loading

Description

DCA warnings were missing when methods call return in some places.

Changes e2e test runner to use file checker in tests with category runs.

Fixes #5666

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@esdrubal esdrubal added compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen DCA Everything to do with Dead Code Analysis labels Mar 13, 2024
@esdrubal esdrubal self-assigned this Mar 13, 2024
@github-actions GitHub Actions
Copy link

Benchmark for c0ee02c

Click to view benchmark
Test Base PR %
code_action 5.4±0.02ms 5.5±0.03ms +1.85%
code_lens 288.0±11.00ns 290.1±13.43ns +0.73%
compile 6.1±0.09s 6.2±0.05s +1.64%
completion 4.8±0.02ms 4.9±0.11ms +2.08%
did_change_with_caching 5.6±0.07s 5.6±0.03s 0.00%
document_symbol 967.7±16.02µs 941.4±36.84µs -2.72%
format 69.7±0.80ms 70.1±0.94ms +0.57%
goto_definition 362.6±5.97µs 361.9±13.22µs -0.19%
highlight 8.8±0.04ms 9.1±0.17ms +3.41%
hover 590.7±7.98µs 610.2±26.27µs +3.30%
idents_at_position 123.0±0.28µs 122.4±1.44µs -0.49%
inlay_hints 713.0±25.41µs 656.3±18.45µs -7.95%
on_enter 485.7±12.64ns 477.3±5.34ns -1.73%
parent_decl_at_position 3.6±0.05ms 3.7±0.04ms +2.78%
prepare_rename 354.4±14.53µs 353.9±6.46µs -0.14%
rename 9.2±0.21ms 9.6±0.18ms +4.35%
semantic_tokens 1059.5±14.85µs 1057.2±18.55µs -0.22%
token_at_position 353.6±1.91µs 346.1±2.30µs -2.12%
tokens_at_position 3.6±0.03ms 3.7±0.02ms +2.78%
tokens_for_file 410.1±3.35µs 406.3±3.46µs -0.93%
traverse 46.5±2.03ms 45.9±2.04ms -1.29%

@github-actions GitHub Actions
Copy link

Benchmark for 69fc978

Click to view benchmark
Test Base PR %
code_action 5.5±0.09ms 5.3±0.32ms -3.64%
code_lens 287.0±3.99ns 296.5±7.29ns +3.31%
compile 6.3±0.05s 6.1±0.08s -3.17%
completion 5.0±0.02ms 4.9±0.15ms -2.00%
did_change_with_caching 5.6±0.06s 5.6±0.07s 0.00%
document_symbol 1007.5±39.92µs 1019.5±25.85µs +1.19%
format 70.9±1.13ms 70.5±0.67ms -0.56%
goto_definition 362.1±7.67µs 365.3±8.70µs +0.88%
highlight 9.1±0.13ms 8.8±0.16ms -3.30%
hover 603.0±18.63µs 673.4±15.03µs +11.67%
idents_at_position 122.2±0.46µs 124.0±1.15µs +1.47%
inlay_hints 672.6±23.71µs 663.6±9.92µs -1.34%
on_enter 492.8±33.29ns 481.8±9.53ns -2.23%
parent_decl_at_position 3.7±0.02ms 3.6±0.03ms -2.70%
prepare_rename 354.9±5.00µs 366.3±6.07µs +3.21%
rename 9.7±0.21ms 9.3±0.23ms -4.12%
semantic_tokens 1035.4±21.27µs 1056.3±13.51µs +2.02%
token_at_position 352.5±2.46µs 353.7±2.03µs +0.34%
tokens_at_position 3.7±0.03ms 3.6±0.03ms -2.70%
tokens_for_file 416.4±1.61µs 412.0±5.07µs -1.06%
traverse 44.5±1.40ms 44.9±1.80ms +0.90%

@esdrubal esdrubal requested a review from a team March 15, 2024 13:02
@esdrubal esdrubal marked this pull request as ready for review March 15, 2024 13:03
ironcev
ironcev previously requested changes Mar 17, 2024
View reviewed changes
Copy link
Member

@ironcev ironcev left a comment

Choose a reason for hiding this comment

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

To the questions on the contains_never method, maybe it can be done as a different PR, but maybe also as the part of this PR, to extend DCA test for uninhabited types and the cases like the one in the code sample provided in the comment where the variable s even if used somewhere later will actually always be dead. I expect our DCA with the change in this PR will already behave like that.

In that case we could also close #3496 with this PR.

sway-core/src/type_system/info.rs Outdated Show resolved Hide resolved
sway-core/src/type_system/info.rs Outdated Show resolved Hide resolved
sway-core/src/type_system/info.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Benchmark for 7a38cb2

Click to view benchmark
Test Base PR %
code_action 5.5±0.04ms 5.3±0.03ms -3.64%
code_lens 297.6±7.20ns 294.9±7.85ns -0.91%
compile 6.0±0.08s 6.0±0.09s 0.00%
completion 5.0±0.03ms 5.0±0.14ms 0.00%
did_change_with_caching 5.6±0.05s 5.5±0.03s -1.79%
document_symbol 950.8±21.61µs 1022.1±42.42µs +7.50%
format 88.2±0.81ms 87.7±1.04ms -0.57%
goto_definition 357.4±8.05µs 357.1±6.18µs -0.08%
highlight 9.2±0.04ms 8.8±0.26ms -4.35%
hover 641.5±50.33µs 643.8±18.54µs +0.36%
idents_at_position 122.1±0.22µs 122.5±0.74µs +0.33%
inlay_hints 664.5±26.31µs 655.1±23.34µs -1.41%
on_enter 497.5±19.64ns 483.3±13.77ns -2.85%
parent_decl_at_position 3.7±0.02ms 3.6±0.03ms -2.70%
prepare_rename 356.8±1.99µs 358.1±5.99µs +0.36%
rename 9.8±0.08ms 9.3±0.05ms -5.10%
semantic_tokens 1018.8±13.29µs 1031.0±15.14µs +1.20%
token_at_position 355.0±2.53µs 355.3±2.56µs +0.08%
tokens_at_position 3.7±0.02ms 3.6±0.03ms -2.70%
tokens_for_file 407.8±3.09µs 408.6±4.31µs +0.20%
traverse 44.6±1.49ms 44.4±1.35ms -0.45%

@esdrubal esdrubal requested review from a team and ironcev April 10, 2024 10:56
@github-actions GitHub Actions
Copy link

Benchmark for 5360553

Click to view benchmark
Test Base PR %
code_action 5.5±0.11ms 5.3±0.11ms -3.64%
code_lens 289.1±7.66ns 299.6±12.27ns +3.63%
compile 6.0±0.05s 6.1±0.10s +1.67%
completion 4.9±0.07ms 4.8±0.02ms -2.04%
did_change_with_caching 6.0±0.07s 6.1±0.09s +1.67%
document_symbol 972.4±86.66µs 953.3±8.64µs -1.96%
format 75.8±1.26ms 76.1±0.71ms +0.40%
goto_definition 372.5±21.17µs 370.7±17.98µs -0.48%
highlight 9.1±0.02ms 8.7±0.57ms -4.40%
hover 599.1±18.38µs 604.9±20.68µs +0.97%
idents_at_position 124.3±1.97µs 124.3±1.30µs 0.00%
inlay_hints 672.6±21.37µs 665.9±44.93µs -1.00%
on_enter 495.9±12.42ns 510.0±15.77ns +2.84%
parent_decl_at_position 3.7±0.04ms 3.6±0.22ms -2.70%
prepare_rename 369.9±3.07µs 368.0±6.61µs -0.51%
rename 9.6±0.15ms 9.2±0.27ms -4.17%
semantic_tokens 1033.2±26.58µs 1040.8±35.21µs +0.74%
token_at_position 370.3±7.43µs 433.2±3.47µs +16.99%
tokens_at_position 3.7±0.08ms 3.5±0.03ms -5.41%
tokens_for_file 412.8±2.77µs 416.6±3.83µs +0.92%
traverse 50.0±1.06ms 50.2±1.32ms +0.40%

@xunilrj
Copy link
Contributor

xunilrj commented Apr 11, 2024

I left some comments because we are only notifying errors "on the next line", and not on the "next expression", if that makes sense. But I have also approved, because your improvements already improved what we have.

xunilrj
xunilrj previously approved these changes Apr 11, 2024
View reviewed changes
@esdrubal esdrubal mentioned this pull request Apr 11, 2024
Open
@esdrubal
Copy link
Contributor Author

Improved the DCA algorithm:

  • marks while body as unreachable when the condition returns TypeInfo::Never.
  • marks functions as not used when we call them with a parameter that is uninhabitable.
  • marks enums as not used when they are instantiated with an uninhabited value.

@github-actions GitHub Actions
Copy link

Benchmark for 1e2fc4b

Click to view benchmark
Test Base PR %
code_action 5.6±0.10ms 5.5±0.02ms -1.79%
code_lens 288.0±8.02ns 336.4±8.59ns +16.81%
compile 6.3±0.07s 6.2±0.04s -1.59%
completion 4.9±0.09ms 5.0±0.11ms +2.04%
did_change_with_caching 6.2±0.09s 6.1±0.06s -1.61%
document_symbol 945.3±7.68µs 1090.4±53.96µs +15.35%
format 77.3±1.25ms 76.6±4.61ms -0.91%
goto_definition 359.7±8.97µs 361.7±4.40µs +0.56%
highlight 9.1±0.28ms 9.1±0.14ms 0.00%
hover 605.2±19.32µs 601.9±13.66µs -0.55%
idents_at_position 122.2±0.56µs 121.0±0.79µs -0.98%
inlay_hints 659.0±23.71µs 672.9±25.36µs +2.11%
on_enter 479.9±16.96ns 483.7±8.42ns +0.79%
parent_decl_at_position 3.7±0.02ms 3.7±0.04ms 0.00%
prepare_rename 357.5±7.49µs 367.1±5.01µs +2.69%
rename 9.7±0.13ms 9.6±0.14ms -1.03%
semantic_tokens 1029.3±7.69µs 1049.2±10.62µs +1.93%
token_at_position 356.4±2.13µs 361.9±1.55µs +1.54%
tokens_at_position 3.7±0.01ms 3.7±0.03ms 0.00%
tokens_for_file 419.5±1.75µs 413.3±4.58µs -1.48%
traverse 51.8±2.59ms 51.7±2.86ms -0.19%

@IGI-111 IGI-111 dismissed ironcev’s stale review April 11, 2024 14:14

Change is done

@IGI-111 IGI-111 enabled auto-merge (squash) April 11, 2024 14:14
@IGI-111 IGI-111 merged commit c34231f into master Apr 11, 2024
34 checks passed
@IGI-111 IGI-111 deleted the esdrubal/dca_warnings branch April 11, 2024 14:36
@github-actions GitHub Actions
Copy link

Benchmark for f7a685a

Click to view benchmark
Test Base PR %
code_action 5.5±0.07ms 5.5±0.03ms 0.00%
code_lens 289.5±7.73ns 292.2±9.00ns +0.93%
compile 6.1±0.09s 6.2±0.09s +1.64%
completion 5.0±0.09ms 5.0±0.07ms 0.00%
did_change_with_caching 6.1±0.07s 6.1±0.04s 0.00%
document_symbol 995.5±18.14µs 1030.1±24.75µs +3.48%
format 76.1±1.86ms 76.1±1.17ms 0.00%
goto_definition 363.6±3.08µs 366.4±3.69µs +0.77%
highlight 9.1±0.14ms 9.2±0.15ms +1.10%
hover 612.4±26.58µs 600.5±13.05µs -1.94%
idents_at_position 121.4±0.64µs 122.8±0.81µs +1.15%
inlay_hints 664.7±15.84µs 665.9±21.48µs +0.18%
on_enter 479.4±12.74ns 488.2±14.03ns +1.84%
parent_decl_at_position 3.7±0.04ms 3.7±0.02ms 0.00%
prepare_rename 358.2±5.76µs 359.7±3.73µs +0.42%
rename 9.6±0.03ms 9.9±0.11ms +3.13%
semantic_tokens 1112.5±15.29µs 1052.1±45.68µs -5.43%
token_at_position 360.7±2.87µs 362.9±3.45µs +0.61%
tokens_at_position 3.8±0.03ms 3.7±0.03ms -2.63%
tokens_for_file 413.7±5.30µs 414.7±3.92µs +0.24%
traverse 50.0±2.61ms 50.0±1.73ms 0.00%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xunilrj xunilrj xunilrj approved these changes

@IGI-111 IGI-111 IGI-111 approved these changes

@ironcev ironcev Awaiting requested review from ironcev

Assignees

@esdrubal esdrubal

Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen DCA Everything to do with Dead Code Analysis
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix DCA divergence use cases.
4 participants
@esdrubal @xunilrj @ironcev @IGI-111