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

make source id optional when auto impl for new encoding #5721

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Mar 12, 2024

Description

This PR makes source_id optional when auto generation code. Specifically some contracts don't necessarily must have contract methods.

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.

Copy link

Benchmark for a0d134a

Click to view benchmark
Test Base PR %
code_action 5.3±0.13ms 5.3±0.10ms 0.00%
code_lens 294.5±16.63ns 285.9±6.40ns -2.92%
compile 6.1±0.05s 6.1±0.07s 0.00%
completion 4.8±0.08ms 4.9±0.11ms +2.08%
did_change_with_caching 5.5±0.02s 5.5±0.03s 0.00%
document_symbol 1032.0±41.58µs 969.1±23.32µs -6.09%
format 70.1±1.33ms 72.5±1.61ms +3.42%
goto_definition 365.8±9.44µs 359.0±6.50µs -1.86%
highlight 8.8±0.16ms 8.8±0.21ms 0.00%
hover 610.8±19.93µs 624.3±21.58µs +2.21%
idents_at_position 122.8±0.73µs 123.7±1.08µs +0.73%
inlay_hints 652.7±23.83µs 659.6±14.24µs +1.06%
on_enter 493.3±12.99ns 482.0±14.44ns -2.29%
parent_decl_at_position 3.6±0.03ms 3.6±0.30ms 0.00%
prepare_rename 362.0±5.89µs 360.7±6.09µs -0.36%
rename 9.3±0.19ms 9.4±0.21ms +1.08%
semantic_tokens 1063.0±8.07µs 1072.3±18.44µs +0.87%
token_at_position 354.6±1.51µs 359.9±2.02µs +1.49%
tokens_at_position 3.6±0.04ms 3.6±0.04ms 0.00%
tokens_for_file 413.1±4.26µs 411.2±3.56µs -0.46%
traverse 44.0±1.67ms 44.5±1.73ms +1.14%

@xunilrj xunilrj requested a review from a team March 12, 2024 11:53
@xunilrj xunilrj self-assigned this Mar 12, 2024
@tritao
Copy link
Contributor

tritao commented Mar 12, 2024

Specifically some contracts don't necessarily must have contract methods.

In theory I think we should always be able to get a valid module id, so even if they have no contracts, we should be able to get a module id. Is there maybe a span on the contract itself we could use?

@xunilrj
Copy link
Contributor Author

xunilrj commented Mar 12, 2024

In theory I think we should always be able to get a valid module id, so even if they have no contracts, we should be able to get a module id. Is there maybe a span on the contract itself we could use?

I changed it to use the module_id from the Module being type_checked.
But I still think we must not expect a module_id to exist in all cases, given that the data model supports None.

@tritao, what do you think?

Copy link

Benchmark for 5021ddb

Click to view benchmark
Test Base PR %
code_action 5.3±0.12ms 5.6±0.15ms +5.66%
code_lens 286.0±7.43ns 293.1±6.47ns +2.48%
compile 6.4±0.15s 6.2±0.10s -3.13%
completion 4.8±0.09ms 5.0±0.06ms +4.17%
did_change_with_caching 5.9±0.06s 5.7±0.07s -3.39%
document_symbol 952.9±17.69µs 1007.9±49.93µs +5.77%
format 70.6±1.01ms 70.7±0.83ms +0.14%
goto_definition 360.2±8.31µs 364.6±8.74µs +1.22%
highlight 8.8±0.16ms 9.1±0.09ms +3.41%
hover 597.6±11.34µs 623.9±20.21µs +4.40%
idents_at_position 124.7±0.66µs 122.5±0.57µs -1.76%
inlay_hints 655.5±15.34µs 674.7±21.84µs +2.93%
on_enter 492.2±9.09ns 496.3±9.27ns +0.83%
parent_decl_at_position 3.6±0.06ms 3.8±0.02ms +5.56%
prepare_rename 364.5±9.88µs 365.6±3.73µs +0.30%
rename 9.2±0.05ms 9.8±0.19ms +6.52%
semantic_tokens 1048.8±39.25µs 1066.1±74.67µs +1.65%
token_at_position 358.9±2.80µs 361.7±5.35µs +0.78%
tokens_at_position 3.6±0.04ms 3.8±0.02ms +5.56%
tokens_for_file 411.4±2.17µs 415.5±3.47µs +1.00%
traverse 47.2±2.29ms 46.3±2.32ms -1.91%

@tritao
Copy link
Contributor

tritao commented Mar 12, 2024

In theory I think we should always be able to get a valid module id, so even if they have no contracts, we should be able to get a module id. Is there maybe a span on the contract itself we could use?

I changed it to use the module_id from the Module being type_checked. But I still think we must not expect a module_id to exist in all cases, given that the data model supports None.

@tritao, what do you think?

New approach looks good. Not sure about None, in my undersanding we should always be compiling some kind of forc pkg, which should always have a valid module id, but without checking further I can't really say.

I think its good enough to merge like like though.

@xunilrj xunilrj force-pushed the xunilrj/source-id-optional-when-auto-impl branch from 0561811 to 7f7775b Compare March 12, 2024 14:16
Copy link

Benchmark for 83dbe54

Click to view benchmark
Test Base PR %
code_action 5.4±0.17ms 5.4±0.16ms 0.00%
code_lens 287.0±5.55ns 288.5±8.45ns +0.52%
compile 6.3±0.12s 6.4±0.08s +1.59%
completion 5.0±0.18ms 5.1±0.25ms +2.00%
did_change_with_caching 6.0±0.09s 5.8±0.05s -3.33%
document_symbol 1061.1±11.81µs 1004.5±25.11µs -5.33%
format 70.1±2.95ms 71.2±1.49ms +1.57%
goto_definition 361.4±6.18µs 366.6±4.63µs +1.44%
highlight 8.8±0.17ms 9.0±0.17ms +2.27%
hover 666.6±23.83µs 655.7±12.36µs -1.64%
idents_at_position 121.5±0.59µs 122.9±1.12µs +1.15%
inlay_hints 659.0±29.56µs 660.9±22.37µs +0.29%
on_enter 488.8±12.15ns 485.3±16.10ns -0.72%
parent_decl_at_position 3.7±0.13ms 3.7±0.15ms 0.00%
prepare_rename 362.7±8.31µs 361.2±6.33µs -0.41%
rename 9.4±0.14ms 9.4±0.33ms 0.00%
semantic_tokens 1212.8±10.28µs 1089.1±21.85µs -10.20%
token_at_position 356.0±2.34µs 360.6±2.57µs +1.29%
tokens_at_position 3.7±0.15ms 3.7±0.15ms 0.00%
tokens_for_file 411.6±2.51µs 411.2±2.57µs -0.10%
traverse 48.2±1.73ms 47.1±2.78ms -2.28%

@xunilrj xunilrj enabled auto-merge (squash) March 12, 2024 15:32
Copy link

Benchmark for ad86589

Click to view benchmark
Test Base PR %
code_action 5.4±0.38ms 5.3±0.19ms -1.85%
code_lens 291.9±12.23ns 287.5±8.32ns -1.51%
compile 6.2±0.14s 6.1±0.10s -1.61%
completion 4.8±0.03ms 4.8±0.14ms 0.00%
did_change_with_caching 5.5±0.02s 5.6±0.04s +1.82%
document_symbol 969.5±20.55µs 1014.3±42.08µs +4.62%
format 71.2±2.51ms 69.7±0.65ms -2.11%
goto_definition 356.2±2.79µs 355.1±6.77µs -0.31%
highlight 8.8±0.27ms 8.7±0.23ms -1.14%
hover 634.0±12.73µs 611.0±18.31µs -3.63%
idents_at_position 121.2±0.40µs 122.2±1.22µs +0.83%
inlay_hints 644.1±26.87µs 654.8±24.62µs +1.66%
on_enter 486.4±23.14ns 489.8±7.66ns +0.70%
parent_decl_at_position 3.6±0.14ms 3.6±0.03ms 0.00%
prepare_rename 357.6±3.53µs 354.7±4.96µs -0.81%
rename 9.3±0.27ms 9.2±0.29ms -1.08%
semantic_tokens 1038.2±23.47µs 1038.7±10.34µs +0.05%
token_at_position 350.0±2.39µs 348.8±2.81µs -0.34%
tokens_at_position 3.6±0.02ms 3.6±0.01ms 0.00%
tokens_for_file 407.3±1.56µs 406.9±1.81µs -0.10%
traverse 46.4±3.69ms 44.1±1.24ms -4.96%

@xunilrj xunilrj merged commit f3309da into master Mar 13, 2024
35 checks passed
@xunilrj xunilrj deleted the xunilrj/source-id-optional-when-auto-impl branch March 13, 2024 13:27
@xunilrj xunilrj mentioned this pull request Mar 13, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants