-
Notifications
You must be signed in to change notification settings - Fork 170
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
feat!: remove dep::
prefix
#4946
base: master
Are you sure you want to change the base?
Conversation
…ibilities when PathKind::Plain encountered, add test to ensure case never encountered in nargo_fmt, update path parsers, update unit tests and test_programs, cargo fmt/clippy
Would be nice if we could maintain some backwards compatibility on this as we'd be nuking basically every noir program out there
Not sure this is true as we can have a crate with a dependency |
…n other errors occur, make backwards compatible
🚀 Deployed on https://66437e82ef96833c07eaba82--noir-docs.netlify.app |
@TomAFrench I made it backwards compatible and added a deprecation warning when R.e. dependency vs. submodule, this test (run from // main.nr
use reexporting_lib::{FooStruct, MyStruct, lib};
mod lib;
use crate::lib::{FooStruct, MyStruct};
fn main() {
let x: FooStruct = MyStruct { inner: 0 };
assert(lib::is_struct_zero(x));
} // lib.nr
struct MyStruct {
inner: Field
}
type FooStruct = MyStruct; // Nargo.toml
[package]
name = "dep_submodule_overlap"
type = "bin"
authors = [""]
compiler_version = ">=0.28.0"
[dependencies]
reexporting_lib = { path = "../../test_libraries/reexporting_lib" } fails with overlapping name errors: error: Duplicate definitions of import with name lib found
┌─ /Users/michaelklein/Coding/rust/noir/test_programs/compile_success_empty/dep_submodule_overlap/src/main.nr:1:44
│
1 │ use reexporting_lib::{FooStruct, MyStruct, lib};
│ --- Second import found here
2 │
3 │ mod lib;
│ --- First import found here
│
error: Duplicate definitions of import with name FooStruct found
┌─ /Users/michaelklein/Coding/rust/noir/test_programs/compile_success_empty/dep_submodule_overlap/src/main.nr:1:23
│
1 │ use reexporting_lib::{FooStruct, MyStruct, lib};
│ --------- First import found here
·
4 │ use crate::lib::{FooStruct, MyStruct};
│ --------- Second import found here
│
error: Duplicate definitions of import with name MyStruct found
┌─ /Users/michaelklein/Coding/rust/noir/test_programs/compile_success_empty/dep_submodule_overlap/src/main.nr:1:34
│
1 │ use reexporting_lib::{FooStruct, MyStruct, lib};
│ -------- First import found here
·
4 │ use crate::lib::{FooStruct, MyStruct};
│ -------- Second import found here so we currently have conflicts between dependencies and submodules and could distinguish with new issue here |
…2) so that it can be used for the '::' prefix later, add deprecation warning
FYI @noir-lang/developerrelations on Noir doc changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behaviour is going to lead to a bunch of footguns/confusing error messages.
// Plain paths are only used to import children modules. It's possible to allow import of external deps, but maybe this distinction is better? | ||
// In Rust they can also point to external Dependencies, if no children can be found with the specified name | ||
resolve_name_in_module( | ||
crate::ast::PathKind::Plain | crate::ast::PathKind::Dep => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't automatically retry looking for an external dependency if we can't resolve locally here. Consider a Noir program with both a submodule and a dependency both called foo
, we can then write:
// foo/lib.nr
pub fn baz() -> Field {
6
}
// bin/main.nr
fn main() -> pub Field {
foo::bar() + foo::baz()
}
mod foo {
pub(crate) fn bar() -> Field {
5
}
}
This compiles completely fine on this branch despite the fact that foo::bar() + foo::baz()
is very confusing as we've got two completely different foo
s here. If I were to define a baz
function within the foo
submodule I'm then silently changing the call to baz
in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check the first section of the path to see if we can resolve it locally, if so then we should commit to that and raise any errors encountered. Only if we don't see any foo
namespace locally should we start searching through dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should fail to compile unless we modified to foo::bar() + ::foo::baz()
This is a different case as we're trying to import the same name into the same namespace twice. The issue we're wanting to solve is two different paths which share a common prefix but we're loading different names from. |
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
…rom '::path', only resolve plain path as external when the first segment isn't found, rename DepPathPrefixDeprecated -> LitDepPathPrefixDeprecated, add distinct parsing case for '::path' with tests, remove test missing a Nargo.toml, add test_programs for overlapping cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test to show that this fails to compile without dep::
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 8a4d138
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to make this a compile_failure
test rather than execution_failure
test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll look into updating the test. as is, skipping the dep::
makes it load everything from the current module and then compilation succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, strange. I can take a look at this next week as this behaviour isn't what I expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the most recent version has foo::bar
and dep::foo::bar
. two foo::bar
's don't conflict, so I've updated the test to conflict on foo
, viz. foo::bar
vs. foo::baz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(you could have been looking at a previous version that had bar
/baz
? I've changed it around several times)
…ed overlap from dep::foo::bar != foo::bar to foo::bar/foo::baz (overlapping module name)
…est (as they're a type of workspace test)
Description
Problem*
Resolves #2307
Summary*
dep::
is unnecessary to distinguish between internal and external crates, so this PR removes it.Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmt
on default settings.