SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
877 stars 45 forks source link

consider having Value not readonly #52

Closed dzmitry-lahoda closed 5 months ago

dzmitry-lahoda commented 2 years ago
  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.
SubhajitS commented 2 years ago

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

dzmitry-lahoda commented 2 years ago
  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
SteveDunn commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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...

dzmitry-lahoda commented 2 years ago

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.

Herdo commented 2 years ago

@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 commented 2 years ago

@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?

SteveDunn commented 5 months ago

I'm going to close this one as:

Thanks for the feedback. If it turns out that the performance overhead can be demonstrated, then we will of course revisit this.