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

reset instances not be accessible for requiresInit types #22898

Closed
arnetheduck opened this issue Nov 1, 2023 · 8 comments · Fixed by #22900
Closed

reset instances not be accessible for requiresInit types #22898

arnetheduck opened this issue Nov 1, 2023 · 8 comments · Fixed by #22900

Comments

@arnetheduck
Copy link
Contributor

arnetheduck commented Nov 1, 2023

Description

type
  X {.requiresInit.} = object
    x: int

var x: X = X(x: 5)

reset(x)

echo x

This snippet defeats the purpose of requiresInit - ie it should not be possible to access the "default"-initialized state of a requiresInit-based object meaning that after reset the variable must no longer be observable or reset must give an error for such types - related to #22895 - the "fix" might cause better codegen but semantically is still unsound.

Nim Version

devel

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

@ringabout
Copy link
Member

Okay, I will fix #22883 differently but keep the better codegen change.

@arnetheduck
Copy link
Contributor Author

Okay, I will fix #22883 differently

this makes sense, but this is truly a language problem - it should not have allowed you to make the fix this way to begin with ;)

@Araq
Copy link
Member

Araq commented Nov 1, 2023

No... reset shall be allowed to reset the memory and ignore the type system. That's how it started, it was a way to "reset" case objects and it wasn't concerned with the integrity of the case objects either. IIRC.

@arnetheduck
Copy link
Contributor Author

No... reset shall be allowed to reset the memory and ignore the type system. That's how it started, it was a way to "reset" case objects and it wasn't concerned with the integrity of the case objects either. IIRC.

well, can't have both: either requiresInit is sound or reset works the way you describe - the fundamentals are at odds here (unless further access to the instance is prohibited, which without some very fancy lifetime tracking will not work across function boundaries). it's not that "it's not concerned" - it removes the reason requiresInit exists so it's very much a concern in this case. These cracks at the edges eventually grow and become sources of major headaches for anyone actually trying to use the feature as advertised - at that point, it's better to simply remove requiresInit.

@Araq
Copy link
Member

Araq commented Nov 1, 2023

That's just your usual "no middle-ground allowed" opinion which is unworkable. In reality effectively every type that benefits from requiresInit can simply not use reset and our usages of it in the stdlib don't cause the violation of invariants.

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Nov 1, 2023

In reality effectively every type that benefits from requiresInit can simply not use reset

that's fine as long as it's enforced by the compiler, which is the point of requiresInit and of this issue

@arnetheduck
Copy link
Contributor Author

usages of it in the stdlib don't cause the violation of invariants.

escape hatches are also fine if the code using them maintains the invariants through other means, but allowing the public reset function to break requiresInit is the problem this issue is about - all such breakage should be explicit (as the compiler no longer can guarantee soundness and the responsibility instead falls on the developer).

Also, as a user of a library, I cannot know if it calls reset in a safe way or not generally - that's why "can simply not use reset" doesn't work: it requires the kind of deep inspection that the compiler does but is infeasible otherwise.

@elcritch
Copy link
Contributor

elcritch commented Nov 3, 2023

One possible solution could be to add a memoryReset or memoryUnsafe effect to reset. Then folks could use fobids to ensure their types aren't reset in a library.

Araq pushed a commit that referenced this issue Nov 5, 2023
fixes #22898
In these cases, the tables/sets are clears or elements are deleted from
them. It's reasonable to suppress warnings because the value is not
accessed anymore, which means it's safe to ignore the warnings.
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 a pull request may close this issue.

4 participants