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: fix nested struct reference type field initialized check. (fix: #15741) #15752

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

shove70
Copy link
Contributor

@shove70 shove70 commented Sep 14, 2022

  1. Fix compiler accepts uninitialized reference in nested structs #15741
  2. Add test.
struct ContainsRef {
	a &int = unsafe { 0 }
	b &int
}

struct Outer {
	c ContainsRef
}

fn main() {
	o := Outer{}
	dump(o)
}

output:

vlib/v/checker/tests/struct_ref_fields_uninitialized_err.vv:11:7: error: reference field `Outer.c.b` must be initialized
    9 | 
   10 | fn main() {
   11 |     o := Outer{}
      |          ~~~~~~~
   12 |     dump(o)
   13 | }

@shove70 shove70 force-pushed the patch-1 branch 8 times, most recently from 9a8c731 to 3ff9456 Compare September 14, 2022 02:24
cmd/tools/vast/vast.v Outdated Show resolved Hide resolved
@shove70 shove70 force-pushed the patch-1 branch 2 times, most recently from 30580e9 to 4fd218f Compare September 14, 2022 02:39
@yuyi98
Copy link
Member

yuyi98 commented Sep 14, 2022

It may be necessary that references in the structure are not initialized by default and must be initialized when used.

@shove70
Copy link
Contributor Author

shove70 commented Sep 14, 2022

It may be necessary that references in the structure are not initialized by default and must be initialized when used.

Oh, may be. sdl/examples, ui module, and many other references have not been initialized.
The work may be in the opposite direction.
:)

@yuyi98
Copy link
Member

yuyi98 commented Sep 14, 2022

@shove70 You did a good work! There are some rules to be clarified here.

@shove70
Copy link
Contributor Author

shove70 commented Sep 14, 2022

@shove70 You did a good work! There are some rules to be clarified here.

Thank you for your encouragement! :)

@shove70 shove70 force-pushed the patch-1 branch 3 times, most recently from dd52364 to 767d1ec Compare September 14, 2022 05:20
@ghost
Copy link

ghost commented Sep 14, 2022

Wouldn't it be better to make unsafe{ nil } the default initialization value for reference field as well as the default initialization for structure fields?

@shove70
Copy link
Contributor Author

shove70 commented Sep 14, 2022

@spytheman

Please help me review these three PRs, because they will affect the CI check of this PR. Thank you very much.

vlang/sdl#406
vlang/ui#482
vlang/ved#149

vlib/v/checker/struct.v Outdated Show resolved Hide resolved
@spytheman
Copy link
Member

It looks good to me, but it effectively requires, that all struct fields of a reference type, require that = unsafe { nil }, which is more of a language change imho, not just a fix.
@medvednikov what do you think?

@medvednikov
Copy link
Member

I'm personally fine with it. I agree it's a big change though.

@medvednikov
Copy link
Member

@shove70 can you make it a warning for now, for a transitionary period?

3 months later we'll make it an error.

@shove70
Copy link
Contributor Author

shove70 commented Sep 14, 2022

medvednikov

OK, this is better. I'll deal with it. :)

@shove70 shove70 force-pushed the patch-1 branch 2 times, most recently from 96b3f92 to e14ce4e Compare September 14, 2022 16:23
@larpon
Copy link
Contributor

larpon commented Sep 15, 2022

@shove70 great job!

I just found this edge case - where I think it's reporting the wrong result:

module main

struct Ref {}

struct EmbedStruct {
    ref &Ref
}

struct Struct {
    EmbedStruct
}

fn main() {
    ref := &Ref{}
    _ := Struct{
        ref: ref
    }
}

Outputs:

t.v:16:7: warning: reference field `Struct.EmbedStruct.ref` must be initialized
   14 | fn main() {
   15 |     ref := &Ref{}
   16 |     _ := Struct{
      |          ~~~~~~~
   17 |         ref: ref
   18 |     }

Which is wrong IMO, since the ref field of the embedded struct is being initialized.

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.

compiler accepts uninitialized reference in nested structs
5 participants