Fix "Typer regression for sum type derivation in paoloboni/binance-scala-client" #24195
Conversation
zielinsky
force-pushed
the
i24093v2
branch
2 times, most recently
from
e728655 to
f32c066
Compare
| /** Returns the decimal String representation of this BigDecimal. | ||
| */ | ||
| override def toString: String = this.bigDecimal.toString | ||
| override def toString(): String = this.bigDecimal.toString |
There was a problem hiding this comment.
Can we have a fix without this change?
There was a problem hiding this comment.
I think this the main part, this fixes the issue
There was a problem hiding this comment.
It fixes the issue because the issue uses BigDecimal. I'm curious if the same issue could arise when defining a custom class that overrides toString and define a ToExpr on it.
There was a problem hiding this comment.
I'm curious if the same issue could arise when defining a custom class that overrides toString and define a ToExpr on it.
Yes, it can.
If we override methods from Java, we should require the override to be non-nullary, however, that wouldn't be backward-compatible. Alternatively, the typer could rewrite the override method from nullary to nilary.
What's the reason for the mismatch in the definition of the toString method in stdlib (e.g. BigInt vs BigDecimal)? I think that we should standardize this.
In any case, this change is invisible to developers, fixes the regression, and is consistent with the Java definition.
There was a problem hiding this comment.
I'm curious if the same issue could arise when defining a custom class that overrides toString and define a ToExpr on it.
Yes, it can.
I'm sure it would. I don't remember why I wrote my comment with if.
What's the reason for the mismatch in the definition of the toString method in stdlib (e.g. BigInt vs BigDecimal)? I think that we should standardize this.
I agree with that and I tried to make the case a long time ago but I've been told it works as designed. This was the last time I brought this topic in a public comment. Regardless, we should acknowledge the bug with ToExpr and Java overrides in a separate issue and have it more minimized know that we fully understand the root of the issue. When that is done, I would be happy to approve this PR to at least close the regression.
3 participants
When compiling code where a Java-defined method is used as a nullary method, the typer performed eta-expansion, which later triggered the
method toString in class BigDecimal does not take parameterserror in some cases. Switching the override to an empty parameter list (toString()) prevents this.Fixes #24093
Revert workaround introduced in #18095