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

Warn on uninitialized isbits-fields in structs? #24943

Open
KristofferC opened this issue Dec 6, 2017 · 9 comments
Open

Warn on uninitialized isbits-fields in structs? #24943

KristofferC opened this issue Dec 6, 2017 · 9 comments

Comments

@KristofferC
Copy link
Member

For example:

julia> struct S
           a::Int
           b::Bool
           S(a::Int, b::Bool) = new(a)
       end

julia> S(1, false)
S(1, true)

Would it make any sense to warn that the struct S is being created with an uninitialized isbits-field?
The problem is that this will act like a well formed type but there is garbage memory in S.b. It seems like an easy mistake: add a field to the struct, forget to update the new call, use garbage memory that might work in 99% of the cases. Perhaps there could be a way to enforce being explicit about using uninitialized isbits field, like new(a, uninitialized) or something...

@nalimilan
Copy link
Member

Now that we have Array{T}(uninitialized, ...) constructors, requiring new(a, uninitialized) makes sense to me.

@StefanKarpinski
Copy link
Member

Or just disallow uninitialized immutable fields. Of course, for fields of type Union{Int, Void} one probably want to just use Void as a default, but it wouldn't hurt for 1.0 to require people to supply the default explicitly.

@KristofferC
Copy link
Member Author

new(a, uninitialized) would have to be a language feature, but I am not too worried about people wanting to saving the ::Uninitialized in their structs.

@danielwe
Copy link
Contributor

I've stumbled upon two cases in the last 10 days where a struct field was unintentionally left uninitialized in packages within a mainstream ecosystem: SciML/PreallocationTools.jl#21 and SciML/DiffEqOperators.jl#519.

Deprecating the current syntax for uninitialized fields in favor of new(..., undef, ...) would be a great improvement. Seems to me that uninitialized data is something that should only be provided when explicitly requested (like for arrays), not something you can get without warning due to a typo while rapidly iterating on a struct layout.

@danielwe
Copy link
Contributor

danielwe commented May 17, 2022

Btw. with respect to the concern in #26764 (comment) about not being able to actually set a field's value to undef, that would keep working as long as the field's type is set to UndefInitializer, right?

Edit: I guess the part that breaks is the ability to set an untyped field to UndefInitializer() rather than #undef

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented May 17, 2022

I've stumbled upon two cases in the last 10 days where a struct field was unintentionally left uninitialized in packages within a mainstream ecosystem: SciML/PreallocationTools.jl#21 and SciML/DiffEqOperators.jl#519.

Before I get flamed for DiffEqOperators having this issue in the "mainstream ecosystem", I just want to note that there's a huge disclaimer in the documentation that it's not.

Screenshot 2022-05-17 081956

I would chalk it down to a failed experiment, at this point almost entirely superseded by MethodOfLines.jl hence there's no major urgency to do do much there (and its JacVecOperator is essentially deprecated in favor of SpraseDiffTools at this point). So, yes it has the issue, but no in no way does that library try to say it's for primetime: it says the opposite in a big black bold note on the first page of the docs.

But yes, I am highly in favor of this. It's fairly easy for a constructor to overtime be dropping off a field accidentally, especially for some optional cache. Requiring an explicit undef would make things a lot easier to maintain.

@danielwe
Copy link
Contributor

I didn't mean to fire any shots, @ChrisRackauckas. I should have dropped you a note before linking the issues here. I apologize for not doing that. I hesitated a little but decided that it's important to show real-world examples to demonstrate that this trap is a problem. The point was emphatically not to call out the specific packages, but rather to show that the current language semantics is problematic.

I did consider adding a clause about both packages being somewhat peripheral in the SciML ecosystem (which is what I meant to refer to by "a mainstream ecosystem") but omitted it for brevity.

@ChrisRackauckas
Copy link
Member

Oh no worries, that comment wasn't necessarily for you. I just wanted to clarify this for the record, since I have more than a few people jump at any mistake like this and say "see, Julia had a bug in a major package, this guy said so", so I've been careful to mention which packages are just development prototypes and not "SciML proper" (though a lot of people seem to treat DiffEqOperators as SciML proper anyways...). So I just wanted to place that disclaimer once I saw it. But yes, sorry to derail the conversation a bit, I'll leave it there.

@staticfloat
Copy link
Member

Shouldn't this be an error? I can't see any reason why we should allow reading from uninitialized memory without explicitly allocating it as such.

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

No branches or pull requests

6 participants