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

Fix scwq instruction and "phantom" error in purity checks #6432

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Aug 18, 2024

Description

This PR fixes #6320 and #6431 by:

  • properly treating the scwq instruction in purity check as a storage writing instruction.
  • not emitting purity issues for __entry functions.

Additionally, the PR:

  • removes purity checks from the type-checking phase. Those checks were:
    • redundant. The same checks were also done at the IR side.
    • incomplete. The checks were covering only the function and method calls, not other kinds of storage access (e.g., intrinsics).
    • inaccurate. The checks were relying on #[storage] attributes, which do not necessary represent actual storage access patterns.
  • extends MetadataManager to be able to store more then one Span per MetadataIndex. E.g., for a function, we can now store the span of the whole function declaration, but in addition, also the span pointing only to the function name.
  • removes two different and overlapping storage access mismatch errors and introduces one expressive diagnostics that points to the exact storage access violations inside of a function (see demo below).

Note that having the purity tests at the IR level means that no purity errors will be reported on non-used functions. This was already the case before, since the complete set of tests was done at the IR level, and only two tests (inaccurately) at the type-checking phase, where errors would be reported even for the non-used functions. The purity guaranty is given for all the compiled code.

Closes #6320.
Closes #6431.

Demo

Before, we had two different error messages, rendered potentially several times per access violation.

Before 01 - Storage attribute access mismatch

Before 02 - Function performs storage read

Now, we have only one error message that points to the access violations and explains them.

After 01 - Pure function cannot access storage

After 02 - Pure function cannot access storage

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.

@ironcev ironcev added bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged compiler: ir IRgen and sway-ir including optimization passes compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: ui Mostly compiler messages labels Aug 18, 2024
@ironcev ironcev self-assigned this Aug 18, 2024
Copy link

Benchmark for d22e678

Click to view benchmark
Test Base PR %
code_action 5.3±0.08ms 5.3±0.11ms 0.00%
code_lens 300.1±6.84ns 302.1±8.02ns +0.67%
compile 2.7±0.03s 2.7±0.04s 0.00%
completion 4.7±0.06ms 4.7±0.08ms 0.00%
did_change_with_caching 2.6±0.05s 2.6±0.06s 0.00%
document_symbol 844.9±18.44µs 923.6±46.60µs +9.31%
format 72.3±0.82ms 67.2±0.78ms -7.05%
goto_definition 339.2±8.33µs 346.5±4.21µs +2.15%
highlight 9.1±0.04ms 9.1±0.05ms 0.00%
hover 488.5±7.64µs 502.5±5.14µs +2.87%
idents_at_position 120.0±0.41µs 119.4±0.77µs -0.50%
inlay_hints 639.8±39.15µs 643.5±10.03µs +0.58%
on_enter 2.6±0.06µs 2.1±0.04µs -19.23%
parent_decl_at_position 3.7±0.03ms 3.7±0.09ms 0.00%
prepare_rename 340.5±7.45µs 344.5±3.53µs +1.17%
rename 9.7±0.20ms 9.4±0.13ms -3.09%
semantic_tokens 1291.8±19.44µs 1282.6±15.20µs -0.71%
token_at_position 348.5±13.28µs 336.1±2.83µs -3.56%
tokens_at_position 3.7±0.06ms 3.7±0.02ms 0.00%
tokens_for_file 409.6±3.79µs 408.0±3.47µs -0.39%
traverse 35.0±0.83ms 34.7±0.86ms -0.86%

Copy link

Benchmark for 89bfeb5

Click to view benchmark
Test Base PR %
code_action 5.3±0.15ms 5.3±0.01ms 0.00%
code_lens 295.9±5.06ns 299.2±13.54ns +1.12%
compile 2.7±0.05s 2.7±0.06s 0.00%
completion 4.7±0.02ms 4.7±0.04ms 0.00%
did_change_with_caching 2.5±0.02s 2.5±0.03s 0.00%
document_symbol 878.3±28.01µs 907.3±30.65µs +3.30%
format 70.6±0.51ms 69.6±0.71ms -1.42%
goto_definition 332.1±8.41µs 345.7±6.17µs +4.10%
highlight 9.1±0.11ms 9.2±0.13ms +1.10%
hover 488.5±6.52µs 496.8±5.96µs +1.70%
idents_at_position 119.9±0.42µs 119.7±1.35µs -0.17%
inlay_hints 634.6±24.73µs 641.6±19.45µs +1.10%
on_enter 2.4±0.09µs 2.0±0.08µs -16.67%
parent_decl_at_position 3.7±0.03ms 3.7±0.03ms 0.00%
prepare_rename 332.9±6.30µs 340.3±3.75µs +2.22%
rename 9.4±0.12ms 9.5±0.09ms +1.06%
semantic_tokens 1146.5±26.89µs 1251.3±9.40µs +9.14%
token_at_position 336.9±2.31µs 337.4±2.15µs +0.15%
tokens_at_position 3.7±0.01ms 3.7±0.01ms 0.00%
tokens_for_file 397.5±1.94µs 396.9±2.11µs -0.15%
traverse 32.8±0.67ms 33.0±0.51ms +0.61%

@ironcev ironcev marked this pull request as ready for review August 18, 2024 21:51
@ironcev ironcev requested review from a team as code owners August 18, 2024 21:51
@ironcev ironcev enabled auto-merge (squash) August 18, 2024 21:51
Copy link
Contributor

@tritao tritao left a comment

Choose a reason for hiding this comment

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

Nice to see this, new approach looks a lot more robust.

Copy link

Benchmark for 7e4dbb2

Click to view benchmark
Test Base PR %
code_action 5.3±0.06ms 5.2±0.05ms -1.89%
code_lens 300.3±9.89ns 302.1±8.65ns +0.60%
compile 2.7±0.05s 2.8±0.05s +3.70%
completion 4.7±0.07ms 4.7±0.05ms 0.00%
did_change_with_caching 2.6±0.06s 2.7±0.04s +3.85%
document_symbol 910.0±22.42µs 939.4±25.21µs +3.23%
format 72.4±1.96ms 67.3±0.71ms -7.04%
goto_definition 349.5±3.61µs 338.4±5.95µs -3.18%
highlight 9.2±0.09ms 9.1±0.09ms -1.09%
hover 502.0±33.72µs 487.4±7.24µs -2.91%
idents_at_position 118.2±0.31µs 118.9±0.54µs +0.59%
inlay_hints 672.2±25.87µs 639.6±20.30µs -4.85%
on_enter 1985.0±107.62ns 1995.5±75.61ns +0.53%
parent_decl_at_position 3.7±0.05ms 3.7±0.02ms 0.00%
prepare_rename 335.3±7.15µs 336.8±4.00µs +0.45%
rename 9.4±0.07ms 9.4±0.10ms 0.00%
semantic_tokens 1221.8±13.64µs 1270.1±11.86µs +3.95%
token_at_position 330.6±5.10µs 337.8±1.92µs +2.18%
tokens_at_position 3.7±0.04ms 3.7±0.08ms 0.00%
tokens_for_file 400.5±6.34µs 406.6±4.45µs +1.52%
traverse 35.0±0.56ms 34.5±0.54ms -1.43%

@ironcev ironcev merged commit 7731c6b into master Aug 19, 2024
43 checks passed
@ironcev ironcev deleted the ironcev/6320-incorrect-function-purity-check branch August 19, 2024 14:18
Copy link

Benchmark for fef8e43

Click to view benchmark
Test Base PR %
code_action 5.4±0.17ms 5.3±0.02ms -1.85%
code_lens 298.5±4.75ns 299.6±7.23ns +0.37%
compile 2.6±0.05s 2.7±0.08s +3.85%
completion 4.7±0.06ms 4.8±0.04ms +2.13%
did_change_with_caching 2.5±0.04s 2.6±0.03s +4.00%
document_symbol 874.9±33.75µs 881.8±9.56µs +0.79%
format 71.2±0.96ms 66.3±0.96ms -6.88%
goto_definition 340.8±7.92µs 368.2±9.41µs +8.04%
highlight 9.1±0.05ms 9.1±0.01ms 0.00%
hover 495.5±7.40µs 517.0±7.04µs +4.34%
idents_at_position 119.5±0.27µs 119.0±0.32µs -0.42%
inlay_hints 647.0±19.81µs 668.7±18.73µs +3.35%
on_enter 2.1±0.03µs 1984.0±80.38ns -5.52%
parent_decl_at_position 3.7±0.01ms 3.8±0.03ms +2.70%
prepare_rename 339.0±7.68µs 342.3±3.14µs +0.97%
rename 9.6±0.19ms 9.4±0.07ms -2.08%
semantic_tokens 1239.3±17.62µs 1250.0±9.73µs +0.86%
token_at_position 335.2±2.09µs 347.3±2.04µs +3.61%
tokens_at_position 3.7±0.02ms 3.8±0.01ms +2.70%
tokens_for_file 403.9±1.84µs 397.4±2.53µs -1.61%
traverse 33.0±0.70ms 33.7±1.54ms +2.12%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: ir IRgen and sway-ir including optimization passes compiler: ui Mostly compiler messages compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
3 participants