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 module name mandatory and private #6271

Merged
merged 9 commits into from
Jul 19, 2024

Conversation

jjcnn
Copy link
Contributor

@jjcnn jjcnn commented Jul 15, 2024

Description

Partial fix of #5498 .

So far the name field in namespace::Module has been public and of type Option<Ident>. This makes little sense, since the name of a module should always have a value, and should retain that value once it is set. This PR fixes this problem.

The visibility and span fields have also been made private. visibility can be read-only, but there is a single instance in which the span field must be set later, so I have added a setter for that field too.

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.

@jjcnn jjcnn requested review from a team as code owners July 15, 2024 21:16
@jjcnn jjcnn marked this pull request as draft July 15, 2024 21:21
Copy link

Benchmark for a00f7e5

Click to view benchmark
Test Base PR %
code_action 5.4±0.74ms 5.0±0.11ms -7.41%
code_lens 339.0±11.56ns 288.7±6.79ns -14.84%
compile 2.6±0.03s 2.6±0.03s 0.00%
completion 4.7±0.06ms 4.6±0.57ms -2.13%
did_change_with_caching 2.5±0.03s 2.5±0.03s 0.00%
document_symbol 894.7±33.69µs 889.6±45.16µs -0.57%
format 73.9±0.97ms 70.5±1.05ms -4.60%
goto_definition 341.3±6.28µs 346.2±9.41µs +1.44%
highlight 9.0±0.11ms 8.7±0.02ms -3.33%
hover 498.9±5.78µs 498.7±6.11µs -0.04%
idents_at_position 118.0±0.29µs 118.5±0.38µs +0.42%
inlay_hints 633.4±25.17µs 632.3±23.20µs -0.17%
on_enter 466.6±21.36ns 484.1±38.40ns +3.75%
parent_decl_at_position 3.7±0.01ms 3.5±0.03ms -5.41%
prepare_rename 336.0±3.95µs 343.2±5.20µs +2.14%
rename 9.2±0.12ms 9.0±0.16ms -2.17%
semantic_tokens 1225.1±12.23µs 1267.2±17.75µs +3.44%
token_at_position 341.2±2.57µs 335.6±3.26µs -1.64%
tokens_at_position 3.7±0.22ms 3.5±0.04ms -5.41%
tokens_for_file 393.9±1.23µs 404.1±8.43µs +2.59%
traverse 37.1±0.84ms 36.8±0.85ms -0.81%

Copy link

Benchmark for 1cb266b

Click to view benchmark
Test Base PR %
code_action 5.3±0.14ms 5.1±0.09ms -3.77%
code_lens 285.1±8.93ns 343.4±22.41ns +20.45%
compile 2.7±0.03s 2.7±0.05s 0.00%
completion 4.7±0.11ms 4.6±0.16ms -2.13%
did_change_with_caching 2.6±0.05s 2.6±0.02s 0.00%
document_symbol 843.2±21.40µs 896.2±23.54µs +6.29%
format 71.7±1.17ms 76.8±3.90ms +7.11%
goto_definition 340.5±29.03µs 346.8±6.22µs +1.85%
highlight 9.0±0.12ms 8.7±0.31ms -3.33%
hover 492.0±7.57µs 501.6±6.42µs +1.95%
idents_at_position 118.6±0.31µs 119.0±0.58µs +0.34%
inlay_hints 639.3±21.72µs 628.1±23.02µs -1.75%
on_enter 492.9±16.21ns 467.6±14.41ns -5.13%
parent_decl_at_position 3.7±0.03ms 3.6±0.08ms -2.70%
prepare_rename 339.0±9.38µs 348.0±9.72µs +2.65%
rename 9.4±0.15ms 9.0±0.25ms -4.26%
semantic_tokens 1258.2±13.50µs 1273.4±16.06µs +1.21%
token_at_position 337.1±5.76µs 340.1±5.00µs +0.89%
tokens_at_position 3.7±0.05ms 3.5±0.05ms -5.41%
tokens_for_file 510.5±2.44µs 399.8±2.55µs -21.68%
traverse 38.6±0.57ms 38.8±1.25ms +0.52%

@jjcnn jjcnn marked this pull request as ready for review July 16, 2024 15:44
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
@JoshuaBatty JoshuaBatty requested a review from a team July 16, 2024 23:20
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
Copy link

Benchmark for c0b0c07

Click to view benchmark
Test Base PR %
code_action 5.5±0.15ms 5.1±0.12ms -7.27%
code_lens 288.5±13.49ns 343.3±12.55ns +18.99%
compile 2.7±0.05s 2.7±0.04s 0.00%
completion 5.0±0.22ms 4.7±0.11ms -6.00%
did_change_with_caching 2.7±0.05s 2.7±0.04s 0.00%
document_symbol 943.9±34.25µs 946.5±28.20µs +0.28%
format 72.7±0.78ms 71.8±1.80ms -1.24%
goto_definition 337.0±3.70µs 340.7±9.23µs +1.10%
highlight 9.3±0.13ms 8.7±0.13ms -6.45%
hover 488.7±10.81µs 496.2±6.96µs +1.53%
idents_at_position 118.9±0.39µs 118.0±0.79µs -0.76%
inlay_hints 648.6±21.76µs 641.8±16.38µs -1.05%
on_enter 488.2±31.64ns 468.6±12.82ns -4.01%
parent_decl_at_position 3.8±0.45ms 3.6±0.07ms -5.26%
prepare_rename 338.9±9.20µs 342.5±17.67µs +1.06%
rename 9.8±0.17ms 9.0±0.17ms -8.16%
semantic_tokens 1260.3±20.95µs 1286.9±15.83µs +2.11%
token_at_position 341.6±6.28µs 332.4±5.40µs -2.69%
tokens_at_position 3.7±0.04ms 3.6±0.06ms -2.70%
tokens_for_file 401.0±3.96µs 406.9±4.61µs +1.47%
traverse 38.7±1.17ms 39.5±0.44ms +2.07%

@IGI-111 IGI-111 requested a review from a team July 18, 2024 14:54
Copy link

Benchmark for 7dd3dcb

Click to view benchmark
Test Base PR %
code_action 5.0±0.14ms 5.0±0.11ms 0.00%
code_lens 285.1±6.77ns 286.4±9.95ns +0.46%
compile 2.6±0.03s 2.6±0.02s 0.00%
completion 4.5±0.03ms 4.5±0.07ms 0.00%
did_change_with_caching 2.6±0.02s 2.6±0.03s 0.00%
document_symbol 834.4±20.16µs 886.2±42.97µs +6.21%
format 68.5±0.58ms 68.6±3.48ms +0.15%
goto_definition 331.2±4.56µs 349.3±18.38µs +5.46%
highlight 8.6±0.12ms 8.7±0.12ms +1.16%
hover 486.7±6.35µs 500.1±6.08µs +2.75%
idents_at_position 118.1±0.50µs 119.5±0.48µs +1.19%
inlay_hints 614.8±23.64µs 629.4±33.84µs +2.37%
on_enter 466.2±5.62ns 460.5±36.27ns -1.22%
parent_decl_at_position 3.5±0.04ms 3.5±0.05ms 0.00%
prepare_rename 333.3±8.45µs 341.4±7.02µs +2.43%
rename 8.9±0.11ms 9.0±0.12ms +1.12%
semantic_tokens 1187.0±16.97µs 1271.3±31.96µs +7.10%
token_at_position 332.3±3.13µs 326.5±3.29µs -1.75%
tokens_at_position 3.5±0.03ms 3.5±0.06ms 0.00%
tokens_for_file 397.6±3.93µs 402.8±2.93µs +1.31%
traverse 36.7±0.79ms 37.7±0.99ms +2.72%

@IGI-111 IGI-111 enabled auto-merge (squash) July 19, 2024 11:54
Copy link

Benchmark for ef00e28

Click to view benchmark
Test Base PR %
code_action 5.0±0.10ms 5.2±0.08ms +4.00%
code_lens 287.0±8.06ns 297.1±36.27ns +3.52%
compile 2.8±0.03s 2.8±0.04s 0.00%
completion 4.5±0.04ms 4.7±0.02ms +4.44%
did_change_with_caching 2.6±0.05s 2.6±0.03s 0.00%
document_symbol 848.1±12.04µs 928.4±89.62µs +9.47%
format 70.2±1.39ms 68.3±0.47ms -2.71%
goto_definition 336.0±7.54µs 354.4±5.10µs +5.48%
highlight 8.7±0.23ms 9.1±0.19ms +4.60%
hover 491.9±4.47µs 514.0±5.35µs +4.49%
idents_at_position 120.2±0.78µs 121.7±1.15µs +1.25%
inlay_hints 624.4±26.76µs 687.7±28.15µs +10.14%
on_enter 458.0±14.28ns 480.9±13.81ns +5.00%
parent_decl_at_position 3.5±0.03ms 3.7±0.03ms +5.71%
prepare_rename 335.4±6.13µs 351.3±7.97µs +4.74%
rename 8.9±0.20ms 9.3±0.02ms +4.49%
semantic_tokens 1253.6±9.26µs 1304.4±14.60µs +4.05%
token_at_position 326.9±2.00µs 350.5±5.37µs +7.22%
tokens_at_position 3.5±0.03ms 3.8±0.04ms +8.57%
tokens_for_file 403.1±2.63µs 407.2±3.84µs +1.02%
traverse 38.1±1.13ms 38.2±0.90ms +0.26%

@IGI-111 IGI-111 merged commit de85361 into master Jul 19, 2024
39 checks passed
@IGI-111 IGI-111 deleted the jjcnn/encapsulate-namespace-module branch July 19, 2024 22:51
esdrubal pushed a commit that referenced this pull request Aug 13, 2024
## Description

Partial fix of #5498 .

So far the `name` field in `namespace::Module` has been public and of
type `Option<Ident>`. This makes little sense, since the name of a
module should always have a value, and should retain that value once it
is set. This PR fixes this problem.

The `visibility` and `span` fields have also been made private.
`visibility` can be read-only, but there is a single instance in which
the `span` field must be set later, so I have added a setter for that
field too.


## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: IGI-111 <igi-111@protonmail.com>
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.

4 participants