Implement hierarchical trait map and namespace lookups. #6516

tritao · 2024-09-09T13:05:34Z

Description

This PR implements a hierarchical-based trait map and namespace lookup system while type checking.

Previously we added the concept of lexical concepts, which store all the names in a tree-based hierarchy.
But currently we still rely on a single "global" namespace, which is maintained by the type check context as we go down the tree. Thus, so far, all trait and namespace lookup have relied on looking up in that single namespace.

The main idea here is to be able to just rely on the names existing in the lexical scopes, and walk up those lexical scope chains as we need to lookup any name or trait.

The logic is split into these two commits:

Implement hierarchical trait map lookups.

Implement hierarchical namespace lookups.

This PR still does not remove that cloning, which will be done in a separate future PR.

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-11-18T01:44:01Z

CodSpeed Performance Report

Merging #6516 will not alter performance

_{Comparing tritao:hierarchical-lookups-typechecker (3029139) with master (519f208)}

Summary

✅ 22 untouched benchmarks

sdankel · 2024-11-20T12:11:40Z

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

+        self.get_items_for_type(engines, type_id)
+            .into_iter()
+            .filter_map(|item| match item {
+                ResolvedTraitImplItem::Parsed(_) => todo!(),


nit: I see that this was just moved from another file, but I think we should replace the todo! with something else, like None

nit: I see that this was just moved from another file, but I think we should replace the todo! with something else, like None

I don't think it should return None in this case because that would be a silent failure to the dev. I think maybe an unreachable here probably makes the most sense, and this code is not really designed to be called for parsed declarations.

jjcnn

Overall it looks good, but I think there's a common pattern that can be abstracted out, and there's a nagging feeling at the back of my mind that you've introduced a shadowing issue. Let me know what you think - I might just be overthinking things.

jjcnn · 2024-11-26T11:52:59Z

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

+        let mut all_impld_traits: BTreeSet<(Ident, TypeId)> = Default::default();
+        while let Some(lexical_scope) = lexical_scope_opt {
+            all_impld_traits.extend(
+                lexical_scope
+                    .items
+                    .implemented_traits
+                    .get_implemented_traits(type_id, engines),
+            );


Isn't there a shadowing issue here? Something like if a type implements two different traits with the same name but in different scopes, and then we refer to the type using the trait name.

My Rust skills aren't quite good enough to come up with a counter-example, but if there is a shadowing issue then it won't show itself until you eliminate the cloning of trait maps anyway.

Isn't there a shadowing issue here? Something like if a type implements two different traits with the same name but in different scopes, and then we refer to the type using the trait name.

My Rust skills aren't quite good enough to come up with a counter-example, but if there is a shadowing issue then it won't show itself until you eliminate the cloning of trait maps anyway.

I think there should be no shadowing issue since the return is a list of traits, I asked @esdrubal for confirmation and he thinks at most we can get an ambuiguity error due to more traits being returned, which is what should happen I think.

I suppose my question really is: Is there a case where this algorithm will throw an ambiguity error, but where shadowing actually resolves the ambiguity? I'm not sure, because I'm not sure a trait implementation can shadow another trait implementation.

TraitMap::insert is the method responsible for checking that we don't shadow other trait implementations.
TraitMap::insert can insert more methods for impl selfs and should throw an error when methods with the same name are added. However, TraitMap::insert and other methods were not included in this PR. I am waiting for this to get merged so I can start working on those.

jjcnn · 2024-11-26T12:20:53Z

sway-core/src/semantic_analysis/ast_node/expression/typed_expression.rs

+        while let Some(lexical_scope) = lexical_scope_opt {
+            let result = Self::find_subfield_type_helper(
+                lexical_scope,
+                handler,
+                engines,
+                namespace,
+                base_name,
+                projections,
+            )?;
+            if let Some(result) = result {
+                return Ok(result);
+            }
+            if let Some(parent_scope_id) = lexical_scope.parent {
+                lexical_scope_opt = module.get_lexical_scope(parent_scope_id);
+            } else {
+                lexical_scope_opt = None;
+            }
+        }


The pattern

let mut lexical_scope_opt = ...; while let Some(lexical_scope) = lexical_scope_opt { ... if let Some(parent_scope_id) = lexical_scope.parent { lexical_scope_opt = module.get_lexical_scope(parent_scope_id); } else { lexical_scope_opt = None; } }

is repeated in a couple of places, and seems to capture a structural aspect of hierarchical lexical scopes. Is there a way to abstract this pattern out (ideally so that it is located in the namespace module somewhere), so that traversing the scopes is kept separate from what we are searching for?

jjcnn · 2024-11-26T12:42:45Z

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

+        let mut lexical_scope_opt = Some(self.current_lexical_scope());
+        let mut vec = vec![];
+        while let Some(lexical_scope) = lexical_scope_opt {
+            vec.extend(
+                lexical_scope
+                    .items
+                    .implemented_traits
+                    .get_items_for_type(engines, type_id),
+            );
+            if let Some(parent_scope_id) = lexical_scope.parent {
+                lexical_scope_opt = self.get_lexical_scope(parent_scope_id);
+            } else {
+                lexical_scope_opt = None;
+            }
+        }


The pattern occurs again here.

jjcnn · 2024-11-26T12:43:03Z

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

+        let mut lexical_scope_opt = Some(self.current_lexical_scope());
+        while let Some(lexical_scope) = lexical_scope_opt {
+            let result = lexical_scope
+                .items
+                .resolve_symbol(handler, engines, symbol)?;
+            if let Some(result) = result {
+                return Ok(result);
+            }
+            if let Some(parent_scope_id) = lexical_scope.parent {
+                lexical_scope_opt = self.get_lexical_scope(parent_scope_id);
+            } else {
+                lexical_scope_opt = None;
+            }
+        }


Same pattern again.

jjcnn · 2024-11-26T12:43:29Z

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

+        let mut lexical_scope_opt = Some(module.current_lexical_scope());
+        let mut all_impld_traits: BTreeSet<(Ident, TypeId)> = Default::default();
+        while let Some(lexical_scope) = lexical_scope_opt {
+            all_impld_traits.extend(
+                lexical_scope
+                    .items
+                    .implemented_traits
+                    .get_implemented_traits(type_id, engines),
+            );
+            if let Some(parent_scope_id) = lexical_scope.parent {
+                lexical_scope_opt = module.get_lexical_scope(parent_scope_id);
+            } else {
+                lexical_scope_opt = None;
+            }


Same pattern again.

tritao self-assigned this Sep 23, 2024

Implement hierarchical trait map lookups.

4043fb5

tritao force-pushed the hierarchical-lookups-typechecker branch from dc00260 to 60db3a9 Compare November 18, 2024 01:26

Implement hierarchical namespace lookups.

486df45

tritao force-pushed the hierarchical-lookups-typechecker branch from 60db3a9 to 486df45 Compare November 18, 2024 01:28

tritao changed the title ~~Implement hierarchical namespace lookups.~~ Implement hierarchical trait map and namespace lookups. Nov 18, 2024

tritao added the compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen label Nov 18, 2024

tritao marked this pull request as ready for review November 18, 2024 01:44

tritao requested review from a team as code owners November 18, 2024 01:44

tritao added 2 commits November 18, 2024 11:28

Merge branch 'master' into hierarchical-lookups-typechecker

397e879

Merge branch 'master' into hierarchical-lookups-typechecker

3029139

sdankel reviewed Nov 20, 2024

View reviewed changes

esdrubal approved these changes Nov 26, 2024

View reviewed changes

jjcnn requested changes Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement hierarchical trait map and namespace lookups. #6516

Implement hierarchical trait map and namespace lookups. #6516

tritao commented Sep 9, 2024 •

edited

Loading

codspeed-hq bot commented Nov 18, 2024 •

edited

Loading

sdankel Nov 20, 2024

tritao Nov 25, 2024

jjcnn left a comment

jjcnn Nov 26, 2024

tritao Nov 26, 2024

jjcnn Nov 26, 2024

esdrubal Nov 27, 2024

jjcnn Nov 26, 2024 •

edited

Loading

jjcnn Nov 26, 2024

jjcnn Nov 26, 2024

jjcnn Nov 26, 2024

Implement hierarchical trait map and namespace lookups. #6516

Are you sure you want to change the base?

Implement hierarchical trait map and namespace lookups. #6516

Conversation

tritao commented Sep 9, 2024 • edited Loading

Description

Checklist

codspeed-hq bot commented Nov 18, 2024 • edited Loading

Merging #6516 will not alter performance

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjcnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjcnn Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritao commented Sep 9, 2024 •

edited

Loading

codspeed-hq bot commented Nov 18, 2024 •

edited

Loading

jjcnn Nov 26, 2024 •

edited

Loading