Implements fully working hierarchical lexical scopes. #6784

esdrubal · 2024-12-11T11:01:20Z

Description

This PR implements the missing elements for the hierarchical lexical scopes.
TypeCheckContext no longer uses cloned namespaces. Instead, we push and pop lexical scopes.
For this to work multiple places that used current_items now either use root_items or traverse the lexical scopes.

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).
- If my change requires substantial documentation changes, I have requested support from the DevRel team
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.

codspeed-hq · 2024-12-12T15:28:06Z

CodSpeed Performance Report

Merging #6784 will degrade performances by 58.21%

_{Comparing esdrubal/hierarchical_lexical_scopes (c47a31f) with master (faf399e)}

Summary

❌ 2 regressions
✅ 20 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`master`	`esdrubal/hierarchical_lexical_scopes`	Change
❌	`completion`	21 ms	23.6 ms	-11.38%
❌	`hover`	1.8 ms	4.3 ms	-58.21%

xunilrj · 2024-12-30T12:21:15Z

sway-core/src/semantic_analysis/namespace/lexical_scope.rs

@@ -36,14 +36,14 @@ impl ResolvedFunctionDecl {
    }
 }

-pub(super) type SymbolMap = im::OrdMap<Ident, ResolvedDeclaration>;
-pub(super) type SymbolUniqueMap = im::OrdMap<IdentUnique, ResolvedDeclaration>;
+pub(super) type SymbolMap = HashMap<Ident, ResolvedDeclaration>;


Is HashMap here fine?

It should be fine because we are sorting each iterator before using them.

You should add a comment that justifies the usage here I think.

I tried to use OrdMap but it introduces a significant performance drop:

Benchmark master esdrubal/hierarchical_lexical_scopes Change

❌ compile 5.3 s 8.1 s -34.4%

tritao · 2025-01-08T14:56:36Z

sway-core/src/semantic_analysis/ast_node/declaration/auto_impl.rs

+                unreachable!();
+            };
+
+            ctx.insert_trait_implementation(


Can you add a comment explaining why this is now necessary?

tritao · 2025-01-08T16:19:54Z

sway-core/src/semantic_analysis/namespace/module.rs

@@ -379,6 +416,14 @@ impl Module {
            })
            .collect::<Vec<_>>()
    }
+
+    pub fn get_impl_spans_for_decl(&self, engines: &Engines, ty_decl: &TyDecl) -> Vec<Span> {


Wondering if we should move this to some other file.

… structures.

…ing_unifications

…on_ctx.namespace lexical scopes.

This optimization is now removed from the TraitMAp because it breaks the insert_implementation_for_type of mutable references.

This reverts commit 8d57419.

esdrubal added code quality compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Dec 11, 2024

esdrubal self-assigned this Dec 11, 2024

esdrubal force-pushed the esdrubal/hierarchical_lexical_scopes branch from 920ca89 to aaed7cc Compare December 12, 2024 15:09

esdrubal force-pushed the esdrubal/hierarchical_lexical_scopes branch 2 times, most recently from c5f0f07 to 62fd6dd Compare December 12, 2024 15:47

esdrubal marked this pull request as ready for review December 13, 2024 09:22

esdrubal requested review from a team as code owners December 13, 2024 09:22

xunilrj reviewed Dec 30, 2024

View reviewed changes

esdrubal force-pushed the esdrubal/hierarchical_lexical_scopes branch 2 times, most recently from 792e419 to 8d57419 Compare January 7, 2025 10:46

esdrubal requested review from a team as code owners January 7, 2025 17:09

esdrubal force-pushed the esdrubal/hierarchical_lexical_scopes branch 2 times, most recently from 90c1d89 to f2f0b31 Compare January 8, 2025 10:05

tritao reviewed Jan 8, 2025

View reviewed changes

tritao previously approved these changes Jan 8, 2025

View reviewed changes

esdrubal added 10 commits January 9, 2025 13:10

Removes TypeCheckContext.scoped namespace cloning.

e9bec54

Replaces currrent_items usage with root_items where possible.

2f672e8

Refactors TraitMap to conform with lexical scopes and removes im data…

1c778dc

… structures.

Use method application callpath because it contains beter span.

78ca358

Fixes auto_impl scope usage.

ab323ca

Minor fixes.

838cd96

Changes to not use im data structures inside lexical scope.

78df26c

Fix so new lexical scopes share the same symbols_unique_while_collect…

0e3b0b0

…ing_unifications

Fixes resolve_symbol to traverse more scopes in case of error.

dae76bf

scoped_and_lexical_scope_id now enters ctx.namespace and ctx.collecti…

0f6df18

…on_ctx.namespace lexical scopes.

esdrubal added 17 commits January 9, 2025 13:10

Adds visitor_parent to LexicalScope.

216f1cd

Removes unused imports.

c8b14a8

Changes insert_symbol to use lexical scopes properly.

08e8242

Changes some places to use root_items and resolve_symbol.

ec83b67

Minor fixes.

cf368e9

Removes is_impl_type_changeable optimization.

34a1d47

This optimization is now removed from the TraitMAp because it breaks the insert_implementation_for_type of mutable references.

Adds missing parameter.

630278b

Cargo clippy fixes.

d15b2df

Updates snapshot with metadata changes.

e757fba

Use OrdMap instead of HashMap and sorting.

2af2fc5

Constant cannot be shadowed error points to last const declaraation.

f77b6d8

Updates configurable_dedup_decode snapshot.

17815ae

Revert "Use OrdMap instead of HashMap and sorting."

86010ff

This reverts commit 8d57419.

Adds comment explaining why we use HashMap instead of OrdMap.

3007353

Fixes test.

5de3420

Fixes flaky test snapshot.

2eefab0

Fixes non deterministic method.

c47a31f

esdrubal dismissed tritao’s stale review via c47a31f January 9, 2025 13:43

esdrubal force-pushed the esdrubal/hierarchical_lexical_scopes branch from b18daec to c47a31f Compare January 9, 2025 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements fully working hierarchical lexical scopes. #6784

Implements fully working hierarchical lexical scopes. #6784

esdrubal commented Dec 11, 2024 •

edited

Loading

codspeed-hq bot commented Dec 12, 2024 •

edited

Loading

xunilrj Dec 30, 2024

esdrubal Jan 6, 2025

IGI-111 Jan 6, 2025

esdrubal Jan 7, 2025

tritao Jan 8, 2025 •

edited

Loading

tritao Jan 8, 2025

Implements fully working hierarchical lexical scopes. #6784

Are you sure you want to change the base?

Implements fully working hierarchical lexical scopes. #6784

Conversation

esdrubal commented Dec 11, 2024 • edited Loading

Description

Checklist

codspeed-hq bot commented Dec 12, 2024 • edited Loading

CodSpeed Performance Report

Merging #6784 will degrade performances by 58.21%

Summary

Benchmarks breakdown

xunilrj Dec 30, 2024

Choose a reason for hiding this comment

esdrubal Jan 6, 2025

Choose a reason for hiding this comment

IGI-111 Jan 6, 2025

Choose a reason for hiding this comment

esdrubal Jan 7, 2025

Choose a reason for hiding this comment

tritao Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

tritao Jan 8, 2025

Choose a reason for hiding this comment

esdrubal commented Dec 11, 2024 •

edited

Loading

codspeed-hq bot commented Dec 12, 2024 •

edited

Loading

tritao Jan 8, 2025 •

edited

Loading