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

Wrong TyStructDecl::call_path when using glob imports with local shadowing #5500

Closed
6 tasks
ironcev opened this issue Jan 21, 2024 · 0 comments · Fixed by #6044
Closed
6 tasks

Wrong TyStructDecl::call_path when using glob imports with local shadowing #5500

ironcev opened this issue Jan 21, 2024 · 0 comments · Fixed by #6044
Assignees
Labels
bug Something isn't working 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

Comments

@ironcev
Copy link
Member

ironcev commented Jan 21, 2024

#5418 fixed the issue of finding the proper decls in the namespace in case of shadowing, but it turns out we still have issues in the content of the found decls. The concrete example is having TyStructDecl::call_path containing wrong path, namely not the one to the local struct but the one to the shadowed struct, although the rest of the decl contains valid information.

To observe the wrong call_path, we need to debug the compiler. As a sample tryout project, create a new Sway script with an internal library lib.sw containing the following code:

library;

pub struct TestStruct {
    a: u64,
}

Put the following code in main.sw:

script;

mod lib;

use lib::*;

struct TestStruct {
   x: u64,
   y: u64,
}

fn main() {
   let ts = TestStruct { x: 0, y: 0 };
   poke(ts.x);
}

fn poke<T>(_x: T) { }

The program will compile without errors, thus resolving the local TestStruct as fixed in #5418.

To observe the erroneous behavior, go to sway-core/src/type_system/info.rs, to TypeInfo::apply_subfields and add the following debugging lines into the second match arm Some(field) =>:

if decl.call_path.suffix.as_str() == "TestStruct" {
    dbg!(&field.name);
    dbg!(&decl.call_path.prefixes);
    for field in decl.fields.iter() {
        dbg!(&field.name);
    }
    dbg!(&decl.span.as_str());
}

The output will be as follows:

[sway-core/src/type_system/info.rs:1274] &field.name = x
[sway-core/src/type_system/info.rs:1275] &decl.call_path.prefixes = [
    plays,
    lib,    // !!!!!!! Wrong path! This is the path to the lib TestStruct.
]
[sway-core/src/type_system/info.rs:1277] &field.name = x
[sway-core/src/type_system/info.rs:1277] &field.name = y
[sway-core/src/type_system/info.rs:1279] &decl.span.as_str() = "struct TestStruct {\n   x: u64,\n   y: u64,\n}"

Note that the fields are valid, and that also the span points to the local TestStruct.

Comment out the TestStruct in the lib.sw and rebuild the project. This time, as expected, the call_path will contain the valid path to the local TestStruct in main.sw.

[sway-core/src/type_system/info.rs:1274] &field.name = x
[sway-core/src/type_system/info.rs:1275] &decl.call_path.prefixes = [
    plays,
]
[sway-core/src/type_system/info.rs:1277] &field.name = x
[sway-core/src/type_system/info.rs:1277] &field.name = y
[sway-core/src/type_system/info.rs:1279] &decl.span.as_str() = "struct TestStruct {\n   x: u64,\n   y: u64,\n}"

Fixing this issue will also need to check:

  • if there are other struct decl fields which are wrongly populated
  • consistency of other items behind structs:
    • enums
    • impls
    • functions
    • consts
@ironcev ironcev added bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Jan 21, 2024
@xunilrj xunilrj mentioned this issue Apr 25, 2024
22 tasks
IGI-111 pushed a commit that referenced this issue Jun 11, 2024
…Decode (#6044)

## Description

Fixes #5500 . Also fixes the issue that caused one of @xunilrj 's tests
to fail.

The callpaths generated for struct declarations have so far been
incorrect. The `shadowed_glob_imports` test case shows how this causes a
compilation error when there is a name clash between a star imported
struct and a locally declared struct. This causes a number of issues, in
particular that the generated trait impls of `AbiEncode` and `AbiDecode`
can refer to the wrong type or to a type that doesn't exist.

The incorrect paths were the result of a combination of two bugs:
1. The `is_external` flag on the containing modules was often set
incorrectly. In particular, `core` often had `is_external = false`,
leading to the package name being added to the path of anything declared
in `core`.
2. When typechecking a struct/enum declaration we would look up paths in
the current environment before the name of the struct/enum was bound in
the environment. If a type with the same name had already been imported
this lookup would result in a path to the import source module rather
than the path to current module (which is obviously where the
struct/enum is located).

This PR fixes both bugs:
1. When initializing the root namespace we now ensure that all modules
that have already been added are marked with `is_external = true`. This
happens in `Namespace::init_root()`.
2. When typechecking a type declaration we generate the callpath
directly from the name of the type and the path of the current module.
This happens in `CallPath::ident_to_fullpath()`.

Step 1 unfortunately adds an extra cloning step in the `namespace`
module, but I don't think it's avoidable at the moment. To avoid this
extra cloning step we would need to have a better way of building the
external module structure than we currently have. #5498 tracks this and
other issues.

The improved callpath generation should move us a step closer towards
fixing #4700.

## 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).
- [x] 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: Igor Rončević <ironcev@hotmail.com>
Co-authored-by: João Matos <joao@tritao.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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
Development

Successfully merging a pull request may close this issue.

2 participants