Make import_no Optional #1443

BenTheKush · 2023-07-14T21:08:47Z

This PR does 2 things:

Makes ResolvedFile::import_no an Option<usize>
Improves test suite for import logic

This is a first attempt, I imagine there are a few things that can be improved.

Possible issues include:

Some import_nos are set to 0 when no import is used per this conversation; in this case, the import_no should be set to None. However, I imagine others are set to 0 because it is the first import that is performed. I currently just changed all literal import_no: 0 to import_no: Some(0) to get it to type check, but I'm guessing they may actually want to be import_no: None.
@seanyoung you wanted more tests, and in particular for:
1. when no import path is used, and
2. "when a file is resolved via relative paths and not via import paths/maps".
  Could you expand on these more? For (i), how would I set up a case when no import path is used? For (ii), do you mean in an import statement (like import ./blah.sol)? I have a test that does this (imports integrations/polkadots/createpair.sol which has a relative import), but I'm not sure exactly what we want to test.

Signed-off-by: Ben Kushigian <[email protected]>

seanyoung · 2023-07-15T09:13:44Z

src/sema/tests/mod.rs

+            &polkadot,
+            &cache.get_import_path(file.import_no.unwrap()).unwrap().1


Nothing is imported via import path so these asserts should assert!(file.import_no.is_none());

I don't understand, "createpair.sol" is resolved by file resolver by walking over import paths. If I don't add polkadot as an import path to the file resolver then tests fail.

seanyoung

i. when no import path is used, and

Yes, add a test which removes:

assert!(cache.add_import_path(polkadot.as_path()).is_ok());

ii. "when a file is resolved via relative paths and not via import paths/maps".
Could you expand on these more? For (i), how would I set up a case when no import path is used? For (ii), do you mean in an import statement (like import ./blah.sol)? I have a test that does this (imports integrations/polkadots/createpair.sol which has a relative import), but I'm not sure exactly what we want to test.

let path = OsStr::from("examples/polkadot/flipper.sol");
let ns = parse_and_resolve(path, &mut cache, Target::EVM);

seanyoung · 2023-07-15T09:21:29Z

src/file_resolver.rs

@@ -206,7 +206,7 @@ impl FileResolver {
                return Ok(ResolvedFile {
                    full_path,
                    base,
-                    import_no: 0,
+                    import_no: Some(0),


None/copy of parent

seanyoung · 2023-07-15T09:21:47Z

src/file_resolver.rs

@@ -218,12 +218,12 @@ impl FileResolver {
                return Ok(ResolvedFile {
                    full_path,
                    base,
-                    import_no: 0,
+                    import_no: Some(0),


copy of parent import_no

seanyoung · 2023-07-15T09:22:42Z

src/file_resolver.rs

@@ -236,7 +236,7 @@ impl FileResolver {
            return Ok(ResolvedFile {
                full_path,
                base,
-                import_no: 0,
+                import_no: Some(0),


copy of import_no in cache

Where can I get this? I don't think this is accessible through FileResolver (it's in File which is stored in Namespace).

bkushigian · 2023-07-26T23:04:39Z

@seanyoung just wanted to follow up on this PR. I left a couple replies above.

When importing in Solidity, `import "foo";` does not check path relative to the currrent file, only `import "./foo";` does. This work is also a re-write of hyperledger-solang#1443 Signed-off-by: Sean Young <[email protected]>

seanyoung · 2023-07-27T11:53:19Z

@seanyoung just wanted to follow up on this PR. I left a couple replies above.

I've looked at this and wrote a new PR #1465 which solves a number of issues including import_no being optional. Would you mind having a look and see if that works for you.

When importing in Solidity, `import "foo";` does not check path relative to the currrent file, only `import "./foo";` does. This work is also a re-write of hyperledger-solang#1443 Signed-off-by: Sean Young <[email protected]>

…no (#1465) When importing in Solidity, `import "foo";` does not check path relative to the currrent file, only `import "./foo";` does. This work is also a re-write of #1443 Signed-off-by: Sean Young <[email protected]> Co-authored-by: Cyrill Leutwiler <[email protected]>

seanyoung · 2023-08-11T08:35:08Z

Fixed by #1465

…no (#1465) When importing in Solidity, `import "foo";` does not check path relative to the currrent file, only `import "./foo";` does. This work is also a re-write of hyperledger-solang/solang#1443 Signed-off-by: Sean Young <[email protected]> Co-authored-by: Cyrill Leutwiler <[email protected]>

BenTheKush requested review from seanyoung, xermicus and LucasSte as code owners July 14, 2023 21:08

Changed import_no to Option<usize>, added tests

ab7d6a4

Signed-off-by: Ben Kushigian <[email protected]>

BenTheKush force-pushed the benku/option-import-path branch from e1b3f95 to ab7d6a4 Compare July 14, 2023 21:11

seanyoung reviewed Jul 15, 2023

View reviewed changes

seanyoung requested changes Jul 15, 2023

View reviewed changes

Fixed import_nos in 2/3 places

742a201

BenTheKush requested a review from seanyoung July 19, 2023 20:35

seanyoung mentioned this pull request Jul 27, 2023

Relative imports are only done with relative paths and expose import_no #1465

Merged

seanyoung closed this Aug 11, 2023

BenTheKush deleted the benku/option-import-path branch August 15, 2023 04:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make import_no Optional #1443

Make import_no Optional #1443

BenTheKush commented Jul 14, 2023 •

edited

Loading

seanyoung Jul 15, 2023

BenTheKush Jul 18, 2023

seanyoung left a comment

seanyoung Jul 15, 2023

seanyoung Jul 15, 2023

seanyoung Jul 15, 2023

BenTheKush Jul 18, 2023

bkushigian commented Jul 26, 2023

seanyoung commented Jul 27, 2023

seanyoung commented Aug 11, 2023

		&polkadot,
		&cache.get_import_path(file.import_no.unwrap()).unwrap().1

Make import_no Optional #1443

Make import_no Optional #1443

Conversation

BenTheKush commented Jul 14, 2023 • edited Loading

seanyoung Jul 15, 2023

Choose a reason for hiding this comment

BenTheKush Jul 18, 2023

Choose a reason for hiding this comment

seanyoung left a comment

Choose a reason for hiding this comment

seanyoung Jul 15, 2023

Choose a reason for hiding this comment

seanyoung Jul 15, 2023

Choose a reason for hiding this comment

seanyoung Jul 15, 2023

Choose a reason for hiding this comment

BenTheKush Jul 18, 2023

Choose a reason for hiding this comment

bkushigian commented Jul 26, 2023

seanyoung commented Jul 27, 2023

seanyoung commented Aug 11, 2023

BenTheKush commented Jul 14, 2023 •

edited

Loading