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: remove unnecessary struct ref field initialization checks and notifications at map initializing(fix #20245) #20251

Merged
merged 1 commit into from Dec 22, 2023

Conversation

shove70
Copy link
Contributor

@shove70 shove70 commented Dec 22, 2023

  1. Close structs containing references cannot be used in maps #20245
  2. Remove unnecessary notifications
  3. Handle with the implicit creation of map instances later

@spytheman
Copy link
Member

spytheman commented Dec 22, 2023

No, please make a fix for the notice for mut foo := map[string]MyStruct{} only, without turning the notice into an error (yet).

The time for doing this is not now, given that we are still finding edge cases, where the message should not be shown at all.

@spytheman
Copy link
Member

If we do this now (turn the notice into an error), all it would do is force people to use unsafe {} a lot more, even for cases, where it is not needed, like here:

struct MyStruct {
        a &int
        b int
}

fn main() {
        mut foo := unsafe { map[string]MyStruct{} }
        println(foo)
}

As you can see, the map is empty. No instances were initialized, and thus no message should be shown.

@shove70
Copy link
Contributor Author

shove70 commented Dec 22, 2023

OK, I see. My main concern is that map creates instances implicitly:

struct MyStruct {
	a &int
	b int
}

fn main() {
	mut foo := map[string]MyStruct{}
	println((foo['key'].a))
}

outputs:

&nil

I'm going to follow your advice, keep the other "notifications" unchanged, and cancel the "notifications" here,
Then I'll see if map needs to be handled when it's implicitly created

@spytheman
Copy link
Member

My main concern is that map creates instances implicitly

Yes, that is true. I think we need another checker notice/error for that, nudging the user to use m[k] or { some_default } for those cases, where the map value type has references in it.

@shove70
Copy link
Contributor Author

shove70 commented Dec 22, 2023

Yes, that is true. I think we need another checker notice/error for that, nudging the user to use m[k] or { some_default } for those cases, where the map value type has references in it.

Ok, I'll do it. :)

@shove70 shove70 changed the title checker: improved notification messages when struct ref fields are uninitialized(fix #20245) checker: remove unnecessary struct ref field initialization checks and notifications at map initializing(fix #20245) Dec 22, 2023
@spytheman spytheman merged commit dda2b56 into vlang:master Dec 22, 2023
54 checks passed
@shove70 shove70 deleted the shove-patch-1 branch December 22, 2023 12:17
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.

structs containing references cannot be used in maps
2 participants