Forbid reserved variable names for function arguments #11169
Conversation
|
@amtoine will do! Where should I put them? |
what i would do is look for a test involving |
|
@amtoine I mean in folders! I am confused, because there are a bunch of subfolders in |
KAAtheWiseGit
force-pushed
the
def-in
branch
from
829b65d
to
82c5476
Compare
|
Found |
| #[rstest] | ||
| #[case("def test [ in ] {}")] | ||
| #[case("def test [ in: string ] {}")] | ||
| #[case("def test [ nu: int ] {}")] | ||
| #[case("def test [ env: record<> ] {}")] | ||
| #[case("def test [ --env ] {}")] | ||
| #[case("def test [ --nu: int ] {}")] | ||
| #[case("def test [ --in (-i): list<any> ] {}")] | ||
| #[case("def test [ a: string, b: int, in: table<a: int b: int> ] {}")] | ||
| #[case("def test [ env, in, nu ] {}")] | ||
| fn parse_function_signature_name_is_builtin_var(#[case] phrase: &str) { | ||
| let actual = nu!(phrase); | ||
| assert!(actual.err.contains("nu::parser::name_is_builtin_var")) |
There was a problem hiding this comment.
Thanks for the exhaustive test!
There was a problem hiding this comment.
Yep, I discovered rtest and had some fun with it.
| @@ -515,6 +526,19 @@ pub fn parse_def( | |||
| let mut result = None; | |||
|
|
|||
| if let (Some(mut signature), Some(block_id)) = (sig.as_signature(), block.as_block()) { | |||
| for arg_name in &signature.required_positional { | |||
| verify_not_keyword(working_set, &arg_name.name, sig.span); | |||
There was a problem hiding this comment.
This may be a future improvement but could we get a more specific span pointing to the part in the signature that is violating the rule?
There was a problem hiding this comment.
Not without rewriting .as_signature and nu-protocol::signature. Currently the entire module has no notion of spans.
There was a problem hiding this comment.
Ah right at this point the information is gone (in the call site this is somewhat recoverable but yeah for Signature this doesn't make as much sense).
In the future we could maybe move that check up earlier in the parse, but having the error at all should suffice for now.
KAAtheWiseGit
force-pushed
the
def-in
branch
from
6bf45b2
to
85a7ec9
Compare
Works for all arguments and flags. Because the signature parsing doesn't give the spans, it is flags the entire signature. I also added a constant with reserved variable names. Test cases located at `parsing::parse_function_signature_name_is_builtin_var`. Fix nushell#11158.
KAAtheWiseGit
force-pushed
the
def-in
branch
from
85a7ec9
to
24d408b
Compare
There was a problem hiding this comment.
thanks for the tests and working on this @KAAtheWiseGit 🙏
let's land the changes and see how it goes 😉
Works for all arguments and flags. Because the signature parsing doesn't give the spans, it is flags the entire signature. Also added a constant with reserved variable names. Fix nushell#11158.
Works for all arguments and flags. Because the signature parsing doesn't give the spans, it is flags the entire signature. Also added a constant with reserved variable names. Fix nushell#11158.
Works for all arguments and flags. Because the signature parsing doesn't give the spans, it is flags the entire signature.
Also added a constant with reserved variable names.
Fix #11158.