Move ValueClass checking errors to case class scheme #2505
Conversation
| case List(acc1, acc2) if acc1.is(Mutable) => | ||
| ctx.error(ValueClassParameterMayNotBeAVar(clazz, acc1), acc1.pos) | ||
| case List(acc1, acc2) if acc2.is(Mutable) => | ||
| ctx.error(ValueClassParameterMayNotBeAVar(clazz, acc2), acc2.pos) |
There was a problem hiding this comment.
This looks weird to me - a value class may only have one constructor parameter - as such, these cases should never occur and the _ case should tell you this, right?
Was there a reason I'm not seeing for which you needed these?
There was a problem hiding this comment.
Yes, there is.
The "may not be var" error would never show. For class MyClass(var i: Int) extends AnyVal it becomes a List(acc1, acc2) for set and get access, the existing code expects List(acc) which does not happen for vars. A different choice is to skip the more specific error "may not be var" and just keep the "must be exactly one val".
There was a problem hiding this comment.
Ah, I see what you mean!
You're not looking at the class constructor here, to do that you'd call clazz.asClass.primaryConstructor. You're instead looking at the parameter accessors - and vars get a setter which is why you're seeing two items.
There was a problem hiding this comment.
Are you saying, we should change to look at the constructor parameters?
Makes more sense to me. The exisiting check looks at the accessors - any connection to the check for Phantom types?
There was a problem hiding this comment.
Not sure - @nicolasstucki
There was a problem hiding this comment.
Phantoms should not affect this. But you should not assume that the constructor will have 1 parameter. There could potentially be an illegal constructor with more or less parameters. In these cases you can just disregard this error as another one will be emitted for it.
There was a problem hiding this comment.
You should also probably try to get them from clazz.asClass.primaryConstructor. It sounds cleaner.
|
I'm sorry, I didn't find out how I should access the primary constructor parameters to see it there is a |
|
No worries, I'll fix it. |
|
Please note the |
@felixmulder I'm a bit uncertain if the change to parameter checking in
Checking.checkDerivedValueClassis optimal. It appeared to me the expectedList(param)can not occur, vars always become two params (which both are marked mutable).