matrei commented on PR #16322:
URL: https://github.com/apache/grails-core/pull/16322#issuecomment-5695272004
## Review Findings, round 3
Head `7a31802892`, base `8.0.x` at `37f8ca6377`. The branch contains the
base head, so there is nothing to merge. The round-2 findings are all addressed
in `fc1941a96e`: `Float` widened to `Double` now passes, every `castToType`
failure is wrapped with the field name (the `NaN`/`BigDecimal` case has its own
spec), and the staging `Sync` mirrors the plugin's `EXCLUDE` strategy.
What I ran on the head, all green, with `cleanTest` and `--no-build-cache`
so the result XML is from this run:
- `:grails-gsp-core:test`, the whole module: 227 tests, 0 failures.
`GspCompileStaticSpec` is 66 of them.
- `:grails-scaffolding:test`, the whole module: 31 tests, 0 failures.
- `:grails-test-examples-gsp-compile-static:integrationTest`: 7 tests, 0
failures. The welcome page is compiled and listed in `views.properties` as
before.
- `:grails-gsp-core:codeStyle` and `:grails-scaffolding:codeStyle`: no
violations.
- Duplicate check: with a throwaway root `index.gsp` in the example app,
`stageViewsWithWelcomePage` succeeds and the staged file is the app's, not the
profile's. Removed afterwards.
One finding, in the rule the new commit chose for the numeric guard.
### [P2] The exact-binary comparison rejects almost every real value
crossing between `BigDecimal` and `Double`/`Float`
References:
- `grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java:209-228`
(`sameNumericValue`, `exactDecimalValue`)
-
`grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:313-314`
(the `'Double' | 0.1G` and `'BigDecimal' | 0.1d` rows)
- `grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:58`
Round 2 asked for the `Float` to `Double` widening to be compared at binary
precision, which the new floating-point branch does. The commit also switched
the cross-family comparison, `BigDecimal` against `Double` or `Float`, from
decimal strings to the exact binary expansion, and that goes the other way: a
`double` is only ever equal to a decimal fraction when the fraction is dyadic,
so `0.5` and `100.00` pass and everything else fails. I rendered `@{
model="<declared> v"}${v}` on the head:
| declared | supplied | head |
|---|---|---|
| `Double` | `19.99G` | fails, "cannot hold the java.math.BigDecimal 19.99
... without changing it" |
| `double` | `19.99G` | fails |
| `Double` | `1234.56G` | fails |
| `Float` | `19.99G` | fails |
| `Double` | `0.1G` | fails, as the doc now says |
| `BigDecimal` | `19.99d` | fails |
| `BigDecimal` | `0.1f` | fails |
| `BigDecimal` | `19.99f` | fails |
| `Double` | `1.5G` | renders `1.5` |
| `Double` | `100.00G` | renders `100.0` |
These are the two families a Grails application actually mixes: GORM money
properties are `BigDecimal`, measurements are `Double` or `Float`, and a
`Double price` page fed a `BigDecimal` domain property, or the reverse, now
fails at render on almost every row of real data. Neither direction loses
anything a user can observe. Groovy's own conversion agrees: `castToType(0.1d,
BigDecimal)` is `0.1`, `castToType(0.1G, Double)` is `0.1`, `castToType(19.99f,
BigDecimal)` is `19.99`, and plain `BigDecimal b = 0.1d` and `double d =
19.99G` assign without complaint. So the guide's first sentence, "converted the
way a Groovy assignment converts it", is contradicted by the paragraph two
lines below it.
The rule that gives the guide's promise is: a conversion is accepted when
the converted value converts back to the original. For the floating-point
family that is the `Double.compare` branch the commit added. For everything
else it is what round 2 already had, the decimal strings, because a `double`
prints as the shortest decimal that reads back to it and that is also the
decimal Groovy converts it to. Concretely, `sameNumericValue` keeps the new
first branch and goes back to `new
BigDecimal(original.toString()).compareTo(new BigDecimal(converted.toString()))
== 0` for the rest, with the existing `NumberFormatException` catch returning
`false`; `exactDecimalValue` goes away. I ran the PR's spec and the table above
with that in place:
- `GspCompileStaticSpec`: 66 tests, 2 failures, and they are exactly the
`'Double' | 0.1G` and `'BigDecimal' | 0.1d` rows in "a conversion that would
change the value", which encode the exact-binary rule. Moving them to the
"converted the way a Groovy assignment converts it" table with expected `0.1`
is the whole spec change.
- Every other rejection stays rejected: `3_000_000_000L` for `Integer`,
`42.9G` for `Integer`, `70_000` for `Short`, `0.1d` for `Float`,
`Long.MAX_VALUE` for `Double`, `16_777_217` for `Float`,
`9_007_199_254_740_993G` for `Double`, `123456789012345678L` for `Double`,
`NaN` for `Integer`, `1E400` for `Double`, `42.5d` for `Integer`, `1.0E+23G`
for `Long`.
- Every row in the table above renders, `0.1f` for `Double` still renders
`0.10000000149011612`, and `0.30000000000000004d` for `BigDecimal` renders
`0.30000000000000004`.
- The one visible consequence of the rule: the `BigInteger` `10^23` becomes
a `Double` and renders `1.0E23`, where the head rejects it because the nearest
`double` is `99999999999999991611392`. That is the round-trip semantics, the
same one that lets `19.99G` through, and it prints what the user supplied.
The doc paragraph at line 58 then reads along the lines of: "A `BigDecimal`
and a `Double` or `Float` are compared through the decimal the floating-point
value prints as, so `19.99G` satisfies a `Double` declaration and `19.99d` a
`BigDecimal` one, while a value with more digits than the type can carry,
`9007199254740993G` for a `Double`, is rejected. Widening a finite `Float` to
`Double` is accepted; narrowing a `Double` to `Float` is rejected if it loses
precision." The `NaN`/infinity sentence stays. The prototype is not in the
working tree.
### Nit: two rows in the "would change the value" table are Groovy refusals,
not guard rejections
References:
-
`grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:317-318`
`'Double' | new BigDecimal('1E400')` and `'Double' |
Float.NEGATIVE_INFINITY` never reach `sameNumericValue`: `castToType` throws
`GroovyRuntimeException` ("Automatic coercion of java.lang.Float value
-Infinity to double failed. Value is out of range.") and the page reports the
"cannot be converted" message with that cause. The feature's assertions check
only the field name and the two type names, which both messages contain, so the
rows pass under the wrong heading. They belong next to the `Date` case in "a
model value that cannot be converted", or that feature's `e.cause` assertion
should become the discriminator. Cosmetic.
### Confirmed
- The guide's "widening a finite `Float` to `Double`" is precise: Groovy
itself refuses an infinite `Float` for a `Double` (plain `Double d =
Float.NEGATIVE_INFINITY` throws the same `GroovyRuntimeException`), while
`Float.NaN` for `Double` and either infinity for `Float` or `float` pass. The
page inherits Groovy's asymmetry rather than adding one.
- `catch (RuntimeException e)` around `castToType` is right:
`GroovyCastException`, `GroovyRuntimeException` and `NumberFormatException` are
all what it throws, none checked, and the new "non-finite value" feature pins
the `NumberFormatException` cause with the field name in the message.
- `Double.compare` in the floating-point branch handles `-0.0d` for `float`
(spec row 9) and `NaN` for `Double` (row 10) as intended, and the `AtomicLong`
row shows a `Number` subclass with no Groovy cast of its own still goes through
the decimal comparison.
- `duplicatesStrategy = DuplicatesStrategy.EXCLUDE` resolves without an
import in a Groovy build script (Gradle's default imports cover
`org.gradle.api.file`), and the app's views being listed first is what makes
the app's page win, verified above.
- The scaffold template, welcome pages, `ScaffoldedIndexViewModelSpec` and
the staging task are unchanged since round 2, and the full `grails-scaffolding`
and `grails-gsp-core` test tasks pass on the head.
## Verification
- `./gradlew :grails-gsp-core:cleanTest :grails-gsp-core:test
:grails-scaffolding:cleanTest :grails-scaffolding:test
:grails-test-examples-gsp-compile-static:cleanIntegrationTest
:grails-test-examples-gsp-compile-static:integrationTest
:grails-gsp-core:codeStyle :grails-scaffolding:codeStyle --no-build-cache
--continue`: BUILD SUCCESSFUL; 227 + 31 + 7 tests, 0 failures; result XML
timestamps from this run.
- `stageViewsWithWelcomePage --rerun-tasks` twice, once with a throwaway
app-level `index.gsp` (staged copy is the app's) and once without (staged copy
is the profile's). The throwaway page was removed.
- A throwaway `ZzProbeSpec` in `grails-gsp-core` rendered the 33
declared/supplied pairs above through `GroovyPagesTemplateEngine`, once on the
head and once with the round-trip prototype in `GroovyPage.java`; the prototype
run also executed `GspCompileStaticSpec` (66 tests, the 2 expected failures).
Both files restored, `git status` shows no tracked changes.
- Stale `book/index.gsp` classes from the round-one hand test were still in
the example app's `build/gsp-classes` and `views.properties`; removed. Build
output only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
