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

checker: disallow structs with @[params] attribute as mutable function parameters #21206

Merged
merged 9 commits into from May 4, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Apr 6, 2024

Fixes #21205

@spytheman
Copy link
Member

I am not sure if that should work.

@medvednikov what do you think?

@ttytm
Copy link
Member Author

ttytm commented Apr 7, 2024

Looking into it again. It should not work at that point. has_arg is always false at the point of the condition and it would not guard when args are passed as not mutable.

So, if a mutable params struct should be allowed after all, the second example below should trigger an error that the arg would have to be passed as part of a mutable struct.

foo() // should work with a mutable params struct?

foo(a: true) // with the PR it doesn't error if the args param is mutable

mut p := Params{a: true} // Should required when deciding o pass a mutable params struct?
foo(mut p)

@medvednikov
Copy link
Member

@spytheman I agree, I think mutable params simply shouldn't be allowed.

@ttytm ttytm force-pushed the checker/mut-params-struct branch from 072be13 to bc6830c Compare April 25, 2024 13:22
@ttytm
Copy link
Member Author

ttytm commented Apr 25, 2024

Okay, I updated the PR to disallow it when declaring a fn with it.

But I don't fully see through where it would cause problems. Why not allowing it to be handled like any other mutable struct and to be optional?

@ttytm ttytm changed the title checker: allow params struct when checking for mutable args checker: disallow structs with @[params] attribute as mutable function parameters Apr 25, 2024
@spytheman
Copy link
Member

Okay, I updated the PR to disallow it when declaring a fn with it.

But I don't fully see through where it would cause problems. Why not allowing it to be handled like any other mutable struct and to be optional?

For normal non @[params] mut arguments, you have to use mut in all call sites, i.e. do f(mut s). You can not just f(Struct{}) or f(s).

If you allow f() or f(some_field: some_value) for fn f(mut opts Params) {, then you would not have to use mut inside the calls - they will look just like normal calls, that do not pass any mutable state down to f().

That can be valuable, but also inconsistent.

-> I can see it both ways, thus I could not decide how it should behave.

mut c_ := Config{
def: 10
}
c := mut_bar_config(mut c_, 10)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should remain to be allowed, the call has mut c_.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inconsistency, is only when you use the default f(), and f gets a mutable parameter, that was created implicitly. If you pass it explicitly, you and the reader will know where it originated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inconsistency, is only when you use the default f(), and f gets a mutable parameter, that was created implicitly. If you pass it explicitly, you and the reader will know where it originated.

The test used Config (a struct with a @[params] attribute) to test passing mutable structs. That's why I added another test passing a mut struct that isn't a params struct.

Else wouldn't this disagree with alex, disallowing mut params, and lead us where I wanted to go in the second comment? Allowing a params struct like this:

mut p := Params{a: true}
foo(mut p)

Or is what is meant to be done here to only disallow trailing mut params structs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know. @medvednikov what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@[params]
struct Config {}

It's a params struct, so it makes sense that the new mut rule is applied to it as well.

It's fine.

Thanks, great job.

@medvednikov medvednikov merged commit f0abc45 into vlang:master May 4, 2024
56 checks passed
@ttytm ttytm deleted the checker/mut-params-struct branch May 4, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mutable params struct not optional as function argument
3 participants