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

Forbid storage key collisions between storage and StorageMap #6466

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Aug 25, 2024

Description

This PR fixes #6317 by introducing two "storage domains", one for the storage fields defined inside of the storage declaration, and a different one for storage slots generated inside of the StorageMap.

The PR strictly fixes the issue described in #6317 by applying the recommendation proposed in the issue.
A general approach to storage key domains will be discussed as a part of the Configurable and composable storage RFC.

Additionally, the PR:

  • adds expressive diagnostics for the duplicated storage keys warning.
  • removes the misleading internal compiler error that always followed the storage key type mismatch error (see demo below).

Closes #6317.

Breaking Changes

The PR changes the way how storage keys are generated for storage fields. Instead of sha256("storage::ns1::ns2.field_name") we now use sha256((0u8, "storage::ns1::ns2.field_name")).

This is a breaking change for those who relied on the storage key calculation.

Demo

Before, every type-mismatch was always followed by an ICE, because the compilation continued despite the type-mismatch.

Mismatched types and ICE - Before

This is solved now, and the type-mismatch has a dedicated help message.

Mismatched types and ICE - After

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 compiler General compiler. Should eventually become more specific as the issue is triaged breaking May cause existing user code to break. Requires a minor or major release. compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Aug 25, 2024
@ironcev ironcev self-assigned this Aug 25, 2024
Copy link

Benchmark for b830f90

Click to view benchmark
Test Base PR %
code_action 5.0±0.01ms 5.1±0.09ms +2.00%
code_lens 283.6±6.98ns 285.6±7.94ns +0.71%
compile 2.6±0.04s 2.7±0.05s +3.85%
completion 4.6±0.05ms 4.5±0.01ms -2.17%
did_change_with_caching 2.6±0.03s 2.5±0.04s -3.85%
document_symbol 916.9±35.99µs 903.3±29.60µs -1.48%
format 70.4±0.65ms 67.6±1.08ms -3.98%
goto_definition 338.8±5.42µs 345.8±7.19µs +2.07%
highlight 8.7±0.01ms 8.7±0.05ms 0.00%
hover 489.7±4.49µs 498.6±5.54µs +1.82%
idents_at_position 118.0±0.29µs 117.4±0.39µs -0.51%
inlay_hints 622.9±22.41µs 628.0±21.49µs +0.82%
on_enter 2.1±0.05µs 2.0±0.08µs -4.76%
parent_decl_at_position 3.6±0.04ms 3.6±0.04ms 0.00%
prepare_rename 337.1±5.52µs 344.4±8.52µs +2.17%
rename 9.0±0.01ms 9.0±0.14ms 0.00%
semantic_tokens 1244.4±11.46µs 1246.6±8.35µs +0.18%
token_at_position 336.3±2.48µs 335.2±1.45µs -0.33%
tokens_at_position 3.6±0.03ms 3.5±0.01ms -2.78%
tokens_for_file 400.4±2.18µs 404.4±3.37µs +1.00%
traverse 34.1±0.89ms 34.6±0.54ms +1.47%

@ironcev ironcev marked this pull request as ready for review August 25, 2024 20:22
@ironcev ironcev requested review from a team as code owners August 25, 2024 20:22
@ironcev ironcev enabled auto-merge (squash) August 25, 2024 20:22
@ironcev ironcev requested a review from a team August 25, 2024 20:23
Copy link

Benchmark for 4944773

Click to view benchmark
Test Base PR %
code_action 5.1±0.10ms 5.0±0.10ms -1.96%
code_lens 290.9±9.27ns 288.6±9.83ns -0.79%
compile 2.7±0.03s 2.7±0.07s 0.00%
completion 4.5±0.06ms 4.5±0.03ms 0.00%
did_change_with_caching 2.5±0.06s 2.6±0.03s +4.00%
document_symbol 894.6±32.14µs 960.8±15.76µs +7.40%
format 72.3±1.04ms 67.7±0.53ms -6.36%
goto_definition 341.6±7.17µs 341.3±7.31µs -0.09%
highlight 8.7±0.17ms 8.8±0.16ms +1.15%
hover 493.7±6.73µs 496.2±9.74µs +0.51%
idents_at_position 118.6±0.41µs 118.5±0.71µs -0.08%
inlay_hints 631.3±12.00µs 631.4±28.47µs +0.02%
on_enter 2.1±0.05µs 2.0±0.07µs -4.76%
parent_decl_at_position 3.6±0.03ms 3.6±0.03ms 0.00%
prepare_rename 342.0±4.27µs 341.5±7.01µs -0.15%
rename 9.0±0.16ms 9.0±0.22ms 0.00%
semantic_tokens 1228.1±13.18µs 1252.6±19.42µs +1.99%
token_at_position 338.6±3.70µs 334.1±2.17µs -1.33%
tokens_at_position 3.6±0.06ms 3.6±0.02ms 0.00%
tokens_for_file 398.8±2.36µs 403.3±3.21µs +1.13%
traverse 33.6±0.66ms 33.8±0.87ms +0.60%

Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

This being a breaking change, it should be behind a feature flag.

Copy link

Benchmark for f370432

Click to view benchmark
Test Base PR %
code_action 5.2±0.17ms 5.4±0.19ms +3.85%
code_lens 284.3±11.69ns 285.3±12.64ns +0.35%
compile 2.8±0.05s 2.8±0.09s 0.00%
completion 4.8±0.19ms 4.8±0.15ms 0.00%
did_change_with_caching 2.7±0.04s 2.8±0.04s +3.70%
document_symbol 981.3±30.14µs 883.9±23.15µs -9.93%
format 71.3±0.89ms 69.2±0.91ms -2.95%
goto_definition 338.6±6.34µs 338.0±7.68µs -0.18%
highlight 9.0±0.22ms 9.4±0.30ms +4.44%
hover 545.9±5.93µs 491.7±6.54µs -9.93%
idents_at_position 118.0±0.48µs 117.3±0.29µs -0.59%
inlay_hints 629.7±27.39µs 641.6±24.28µs +1.89%
on_enter 2.0±0.04µs 1997.1±84.59ns -0.14%
parent_decl_at_position 3.6±0.07ms 3.8±0.08ms +5.56%
prepare_rename 335.5±5.47µs 333.1±6.80µs -0.72%
rename 9.4±0.21ms 9.5±0.25ms +1.06%
semantic_tokens 1194.1±19.04µs 1209.0±10.56µs +1.25%
token_at_position 330.1±1.51µs 329.3±2.38µs -0.24%
tokens_at_position 3.6±0.08ms 3.8±0.10ms +5.56%
tokens_for_file 396.4±2.61µs 397.4±3.10µs +0.25%
traverse 36.0±0.93ms 36.2±0.83ms +0.56%

@ironcev ironcev disabled auto-merge August 27, 2024 09:41
@ironcev
Copy link
Member Author

ironcev commented Aug 27, 2024

As discussed, converting to draft until we get some initial infrastructure for feature flags as discussed in the RFC on bringing changes to the compiler

@ironcev ironcev marked this pull request as draft August 27, 2024 09:43
Copy link

codspeed-hq bot commented Aug 29, 2024

CodSpeed Performance Report

Merging #6466 will not alter performance

Comparing ironcev/6317-storage-collision (837b10d) with master (1bb80d5)

Summary

✅ 21 untouched benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking May cause existing user code to break. Requires a minor or major release. compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
3 participants