Vogen icon indicating copy to clipboard operation
Vogen copied to clipboard

consider having Value not readonly

Open dzmitry-lahoda opened this issue 2 years ago • 8 comments

  1. readonly is (was) slower in runtime in .net for sure.
  2. C# is more like Rust, than Haskell. so mutable structs are OK. as soon as they are not shared across threads, and they are not as these are copy only. so it is safe and better design to make value mutable and allow more patterns of code to work.
  3. sure, validate on set.

dzmitry-lahoda avatar Jan 23 '22 08:01 dzmitry-lahoda

Hi folks, I am interested to start with this. I am new to open source contribution, so would require some guidance to start off.

SubhajitS avatar Feb 02 '22 15:02 SubhajitS

  1. fork repo and find roslyn generator
  2. find where it adds readonly and get.
  3. find how configuration from attributed propagated into generator
  4. added ReadOnly = False
  5. in place where readonly was added to generated code - do not generated that (if on config)
  6. near getter, generate setter with call to validator
  7. in test case where readonly created, create new non readonly
  8. dotnet test
  9. push into your branch
  10. PR via GitHub

dzmitry-lahoda avatar Feb 02 '22 16:02 dzmitry-lahoda

Thanks @SubhajitS - let me know if you need any help. Thanks also @dzmitry-lahoda . I'm thinking - is it worth just removing readonly from Value altogether and not even having it in config?

SteveDunn avatar Feb 04 '22 06:02 SteveDunn

@dzmitry-lahoda - is your main concern the defensive copies? The Value field is get only so there won't be any defensive copies.

One of my goals is to keep the Value Objects as immutable and as safe from default values as possible, so unless there's a compelling reason, I'd rather not introduce immutability.

SteveDunn avatar Feb 06 '22 20:02 SteveDunn

@SubhajitS - I'm having second thoughts on this one. I've asked on runtime discord server, and there isn't any perf overhead. Would like to be proven wrong though...

SteveDunn avatar Feb 24 '22 13:02 SteveDunn

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#readonly-struct if i want readonly, i may mark stuct to be such. if I do not mark, I would have Value not readonly. i am thinking why to impose readonly on macro level? as it can be decided by consumer of macro (source generator). it will be more like rust approach. also recall ref/out/in modifiers in csharp. so making readonly is kind of opinionated I think.

dzmitry-lahoda avatar Feb 24 '22 23:02 dzmitry-lahoda

@dzmitry-lahoda @SteveDunn The purpose of "value objects" is to be a in-place replacement for primative types. Instead of using Int32, Double, Guid, String, etc, a non-primative type obsessed developer should use value objects. This is the whole purpose of Vogen - as far as I understand it.

Therefore, the value objects should behave as close as possible to the underlying primative type. Being immutable is the most important behavior IMO.

Herdo avatar Aug 29 '22 19:08 Herdo

@SteveDunn A suggestion that came to my mind while reading this: maybe enhance the analyzers to promote the usage of readonly struct instead of a struct as an Information or Warning message?

Herdo avatar Aug 29 '22 19:08 Herdo