-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
breaking,checker: disallow initializing private struct fields outside structs module #21183
breaking,checker: disallow initializing private struct fields outside structs module #21183
Conversation
7596007
to
40766dd
Compare
Some of the remaining errors might require discussion as not all fields of them might be meant for a |
52c43a8
to
47ca871
Compare
c6899c0
to
1ee4450
Compare
pub mut: | ||
sock TcpSocket | ||
mut: | ||
sock TcpSocket | ||
handle int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho the handle
should eventually not be publicly mutable. But to not introduce a big breaking change for how TcpConn structs are currently created, having it pub mut is likely the only way.
1ee4450
to
08c02a9
Compare
@spytheman you have push access to https://github.com/elliotchance/vsql? If not maybe it's okay to comment it's CI step out for now as the developer is not active for the last months.
|
a0ca98a
to
a18ddd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work.
I do not, but @elliotchance is on Discord, so I think we can coordinate a change with a PR. I do not want to disable the CI for it on the main repo, if we can avoid it, since it is a very substantial code base, and it has caught V bugs/regressions before. |
I have a question about this PR... how am I supposed to initialize a private field in a struct from another module, if I can't do it at instantiation time? With this change in place, every module that has a struct with a private field will now have to supply a Either that or some sort of setter method just for those fields. |
Private struct fields are usually not not meant to be initialised from other modules. |
*check with tests above: the tests don't test any other/new functionality just include private fields.
81cc83c
to
ff45e89
Compare
I temporary added a fork of external V apps to the CI. The only one remaining (with more restrictive merge access) is the PR at gitly. |
ff45e89
to
c1f8f83
Compare
*imho `handle` should not be public, but currently required to not produce a big breaking change
c1f8f83
to
fbc69a4
Compare
The I think it is good to go. |
246cc89
to
06b6b9a
Compare
Currently, it's possible to init a struct and assign values to it's private fields. Only accessing fields directly will trigger an error.
Currently following is possible
After the changes an error will be visible on initialization (ref. to added test:
struct_field_private_err.out
). Eventually the change should allow for better accessibility control and improved security.