Skip to content

Commit 9247cd4

Browse files
reid-spencerclaude
andcommitted
[1.16] A rule may carry a computed fix; [1.18] the JS logger override is gone
**1.16**: `mechanicalFix` was `Option[String]` -- a CONSTANT -- so a fix whose replacement depends on what it matched could not be written at all. `quoted-constant-literal` (`constant N: Integer = "5"` -> `5`) is a pure span replacement and was excluded purely for want of a way to say so. `Fix` is a sum type rather than a second field beside the old one, because two fields describing one fix can disagree. The distinction earns its keep: a CONSTANT fix is expressible in the published `Map[String, String]` that RiddlLib and DeprecationCode hand to consumers, and a COMPUTED one is not. So the map keeps only constants and says why, while `validate --fix` -- which has the matched span -- applies both. `deprecationEdits` applies computed fixes too, by slicing the span out of the source it already holds. `"1.50"` becomes `1.50`, not `1.5`. A constant replacement could never have done that, and a parse-and-reprint would have silently dropped the precision the author wrote. **1.18**: `DOMPlatformContext.log` returned a FRESH `SysLogger()` on every call. The known symptom was that `withLogger` could not capture on JS. The unnoticed one is that `Logger` holds per-instance counters, so every `count()` landed on a discarded object and `Logger.summary` reported zero on JS for as long as the override stood. And it returned exactly what the base field is already initialised to -- so deleting it fixes both while changing nothing about default behaviour. `RuleIdLogRenderingTest` moves back to SHARED test scala, where it now passes on JS. That matters more than the line count: the defect was a PLATFORM difference, and a suite that skips the platform it differs on cannot see it return. A new `LoggerIdentityTest` asserts the logger is the same instance across calls and that counts accumulate; both were canary-tested by restoring the override on JS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent d6d995c commit 9247cd4

8 files changed

Lines changed: 174 additions & 25 deletions

File tree

BACKLOG.md

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,7 +1070,12 @@ verification is carried here so it is not repeated.**
10701070
Do NOT simply rename it.
10711071

10721072

1073-
- **[1.16]** **A codemod whose replacement is COMPUTED from the matched text.**
1073+
- ~~**[1.16]** **A codemod whose replacement is COMPUTED from the matched text.**~~**DONE
1074+
2026-08-26.** `Fix` is a sum type (`Constant` | `Computed`), so the published
1075+
`Map[String, String]` keeps only what it can express and `validate --fix` applies both.
1076+
`quoted-constant-literal` now fixes. Original entry follows.
1077+
1078+
**[1.16 — as filed]**
10741079
`RuleId.mechanicalFix` is an `Option[String]` -- a CONSTANT replacement -- which covers
10751080
`prompt-statement` -> `do` and `abstract-type` -> `Anything` and cannot express a fix that
10761081
depends on what it matched.
@@ -1092,7 +1097,11 @@ verification is carried here so it is not repeated.**
10921097
at every layer so no corpus model moves. FORMAT_REVISION 23. Riddlg was the requester.
10931098

10941099

1095-
- **[1.18]** **`withLogger` is a silent no-op on Scala.js.**
1100+
- ~~**[1.18]** **`withLogger` is a silent no-op on Scala.js.**~~**DONE 2026-08-26**: the
1101+
override is deleted. It also zeroed every message counter, which nobody had noticed.
1102+
Original entry follows.
1103+
1104+
**[1.18 — as filed]**
10961105
**Verified**: `DOMPlatformContext.scala:88` overrides `def log: Logger = SysLogger()`, returning
10971106
a FRESH logger on every call, so the logger `PlatformContext.withLogger` swaps into the `logger`
10981107
field is never consulted. `pc.withLogger(CallBackLogger(...)) { ... }` therefore captures nothing

commands/src/main/scala/com/ossuminc/riddl/commands/ValidateCommand.scala

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ package com.ossuminc.riddl.commands
88

99
import com.ossuminc.riddl.command.{Command, CommandOptions}
1010
import com.ossuminc.riddl.commands.find.FindEditor
11-
import com.ossuminc.riddl.language.{Messages, RuleId}
11+
import com.ossuminc.riddl.language.{Fix, Messages, RuleId}
1212
import com.ossuminc.riddl.language.Messages.Messages
1313
import com.ossuminc.riddl.language.parsing.RiddlParserInput
1414
import com.ossuminc.riddl.passes.{PassesResult, Riddl}
@@ -181,7 +181,7 @@ class ValidateCommand(using pc: PlatformContext)
181181
// Every diagnostic is classified, so the report can say what it did NOT fix and why. Their
182182
// design note is the reason this is not optional: "a codemod that silently leaves 40 sites is
183183
// worse than one that fixes none".
184-
val fixable = scala.collection.mutable.ListBuffer.empty[(Messages.Message, RuleId, String)]
184+
val fixable = scala.collection.mutable.ListBuffer.empty[(Messages.Message, RuleId, Fix)]
185185
// Grouped by REASON, listing the rules -- not one line per rule. The reason text is identical
186186
// for every rule lacking a fix, so a line each turned a 20-finding model into 11 lines of the
187187
// same sentence and buried the one that mattered.
@@ -205,10 +205,10 @@ class ValidateCommand(using pc: PlatformContext)
205205
"reported span",
206206
rule.code
207207
)
208-
case Some(replacement) =>
208+
case Some(fix) =>
209209
FindEditor.fileOfSource(m.loc.source) match
210210
case None => skip("not in a file this run can edit", rule.code)
211-
case Some(_) => fixable.append((m, rule, replacement))
211+
case Some(_) => fixable.append((m, rule, fix))
212212
}
213213

214214
def reportSkips(label: String): Unit =
@@ -233,10 +233,15 @@ class ValidateCommand(using pc: PlatformContext)
233233
// `fileOfSource`, NOT `Path.of(loc.source.origin)`: origin is the SHORT name error messages
234234
// render, so treating it as a path works only when the cwd happens to be the model's own
235235
// directory. `find -replace` shipped with exactly that bug.
236-
val edits = fixable.toSeq.flatMap { case (m, rule, replacement) =>
236+
val edits = fixable.toSeq.flatMap { case (m, rule, fix) =>
237237
FindEditor
238238
.fileOfSource(m.loc.source)
239-
.map(file => FindEditor.Edit(file, m.loc.offset, m.loc.endOffset, replacement, rule.code))
239+
.map { file =>
240+
// A COMPUTED fix is applied against the text it matched, which the message's own span
241+
// identifies. `quoted-constant-literal` (`"5"` -> `5`) is only expressible this way.
242+
val matched = m.loc.source.data.slice(m.loc.offset, m.loc.endOffset)
243+
FindEditor.Edit(file, m.loc.offset, m.loc.endOffset, fix(matched), rule.code)
244+
}
240245
}
241246
FindEditor.plan(edits) match
242247
case Left(problems) =>

commands/src/test/scala-jvm-native/com/ossuminc/riddl/commands/ValidateFixTest.scala

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,40 @@ class ValidateFixTest extends AnyWordSpec with Matchers {
129129
}
130130
}
131131

132+
"a COMPUTED fix" should {
133+
134+
"rewrite the span using the text it matched" in {
135+
// [1.16]: `mechanicalFix` was `Option[String]` -- a CONSTANT -- so a fix whose replacement
136+
// depends on what it matched could not be expressed at all. `quoted-constant-literal` is a
137+
// pure span replacement (`"5"` -> `5`) and was excluded purely for want of a way to say so.
138+
val model =
139+
"""domain D is {
140+
| context C is {
141+
| constant Threshold: Integer = "5"
142+
| constant Ratio: Real = "1.50"
143+
| } with { briefly "c" described as "c" }
144+
|} with { briefly "d" described as "d" }
145+
|""".stripMargin
146+
withModel(model) { (run, read) =>
147+
run(ValidateCommand.Options(fix = true))
148+
val after = read()
149+
after must include("constant Threshold: Integer = 5")
150+
// `1.50`, not `1.5`: the replacement is the matched text minus its quotes, so precision
151+
// written by the author survives. A parsed-and-reprinted number would not.
152+
after must include("constant Ratio: Real = 1.50")
153+
after mustNot include("\"5\"")
154+
}
155+
}
156+
157+
"not appear in the constant-replacement map, which cannot carry it" in {
158+
// The published Map[String, String] is handed to consumers that apply replacements blindly.
159+
// A computed fix has no constant text, so omitting it is the honest answer rather than
160+
// inventing one.
161+
RuleId.mechanicalReplacements.keySet mustNot contain(RuleId.QuotedConstantLiteral.code)
162+
RuleId.fixable.keySet must contain(RuleId.QuotedConstantLiteral.code)
163+
}
164+
}
165+
132166
"the fixable set" should {
133167
"contain only rules whose fix is a pure span replacement" in {
134168
// shape-keyword rewrites `flow X is` to `processor X as flow is` -- an insertion elsewhere

language/src/main/scala/com/ossuminc/riddl/language/RuleId.scala

Lines changed: 62 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,52 @@ package com.ossuminc.riddl.language
4242
* @param code
4343
* The stable, published identifier. Kebab-case, subject-prefixed.
4444
* @param mechanicalFix
45-
* Replacement text when this rule's fix is a pure SPAN REPLACEMENT -- the message's `loc` covers
46-
* exactly the offending source and swapping in this text resolves it, touching nothing else.
47-
* `None` when the fix needs a judgement call or a rewrite somewhere other than the span.
45+
* How to rewrite the span when this rule's fix is a pure SPAN REPLACEMENT -- the message's `loc`
46+
* covers exactly the offending source and swapping in the result resolves it, touching nothing
47+
* else. `None` when the fix needs a judgement call or a rewrite somewhere other than the span.
4848
* @param deprecates
4949
* True when the rule reports a deprecated construct. Kept ON THE RULE so the set of deprecations
5050
* is derived rather than listed; see the note on `DeprecationCode.all` above.
5151
*/
52+
/** How a rule's fix produces its replacement text.
53+
*
54+
* A sum type rather than a second field beside `Option[String]`: two fields describing one fix can
55+
* disagree, and this repo keeps recording that shape as a defect.
56+
*
57+
* The distinction is real, not bookkeeping. A CONSTANT fix is expressible in the published
58+
* `Map[String, String]` that `RiddlLib.deprecationEdits` and `DeprecationCode.mechanicalReplacement`
59+
* hand to consumers; a COMPUTED one is not, because it needs the matched text. Keeping them
60+
* separate is what lets that map stay honest about what it can carry instead of silently omitting
61+
* or mis-stating the computed ones.
62+
*/
63+
enum Fix:
64+
65+
/** The span is replaced by this exact text, whatever it matched. */
66+
case Constant(text: String)
67+
68+
/** The span is replaced by `f(matched)` -- the fix depends on what it matched.
69+
*
70+
* `quoted-constant-literal` is the reason this exists: `constant N: Integer = "5"` becomes `5`,
71+
* and the replacement is the matched text minus its quotes. That is a pure span replacement --
72+
* it just is not a constant one, and it was excluded from `--fix` purely for want of a way to
73+
* say so.
74+
*/
75+
case Computed(f: String => String)
76+
77+
/** The replacement for a given matched span. */
78+
def apply(matched: String): String = this match
79+
case Constant(text) => text
80+
case Computed(f) => f(matched)
81+
82+
/** The constant text, when there is one. What the published `Map[String, String]` can carry. */
83+
def constantText: Option[String] = this match
84+
case Constant(text) => Some(text)
85+
case _: Computed => None
86+
end Fix
87+
5288
enum RuleId(
5389
val code: String,
54-
val mechanicalFix: Option[String] = None,
90+
val mechanicalFix: Option[Fix] = None,
5591
val deprecates: Boolean = false
5692
):
5793

@@ -154,17 +190,25 @@ enum RuleId(
154190
// was renamed to DoStatement in 2026-08-25 while its code deliberately was not. Renaming a rule
155191
// is a source change; renaming its code is an API break.
156192
case StateIsRecord extends RuleId("state-is-record", deprecates = true)
157-
case DoStatement extends RuleId("prompt-statement", mechanicalFix = Some("do"), deprecates = true)
193+
case DoStatement extends RuleId("prompt-statement", mechanicalFix = Some(Fix.Constant("do")), deprecates = true)
158194
case SendToInlet extends RuleId("send-to-inlet", deprecates = true)
159195
case BareStringCondition extends RuleId("bare-string-condition", deprecates = true)
160196
case AnonymousNebula extends RuleId("anonymous-nebula", deprecates = true)
161197
case ShapeKeyword extends RuleId("shape-keyword", deprecates = true)
162-
case AbstractType extends RuleId("abstract-type", mechanicalFix = Some("Anything"), deprecates = true)
198+
case AbstractType extends RuleId("abstract-type", mechanicalFix = Some(Fix.Constant("Anything")), deprecates = true)
163199
case SingleAlternation extends RuleId("single-alternation", deprecates = true)
164200
case EntityOptionToIntention extends RuleId("entity-option-to-intention", deprecates = true)
165201
case TypeFirstAggregate extends RuleId("type-first-aggregate", deprecates = true)
166202
case ConnectorOptionToIntention extends RuleId("connector-option-to-intention", deprecates = true)
167-
case QuotedConstantLiteral extends RuleId("quoted-constant-literal", deprecates = true)
203+
// The fix is the matched text minus its surrounding quotes -- `"5"` becomes `5`. A pure span
204+
// replacement that simply is not a CONSTANT one, which is the only reason it was excluded from
205+
// `--fix` until [1.16].
206+
case QuotedConstantLiteral
207+
extends RuleId(
208+
"quoted-constant-literal",
209+
mechanicalFix = Some(Fix.Computed(m => m.stripPrefix("\"").stripSuffix("\""))),
210+
deprecates = true
211+
)
168212

169213
// ---- handler: handlers and their on-clauses ----------------------------------------------
170214
case HandlerNoExecutableStatements extends RuleId("handler-no-executable-statements")
@@ -583,7 +627,18 @@ object RuleId:
583627
lazy val deprecations: Seq[RuleId] = values.filter(_.deprecates).toSeq
584628

585629
/** Every rule whose fix is a pure span replacement, as code -> replacement text. */
630+
/** Rules whose fix is a CONSTANT replacement, as code -> text.
631+
*
632+
* A computed fix cannot appear here -- its replacement depends on the matched text, which a
633+
* `Map[String, String]` has no way to express. Omitting it is the honest answer: a consumer
634+
* reading this map gets replacements it can apply blindly, and `validate --fix` (which has the
635+
* matched span) applies the computed ones itself.
636+
*/
586637
lazy val mechanicalReplacements: Map[String, String] =
638+
values.flatMap(r => r.mechanicalFix.flatMap(_.constantText).map(r.code -> _)).toMap
639+
640+
/** Every rule with any mechanical fix, constant or computed. What `validate --fix` acts on. */
641+
lazy val fixable: Map[String, Fix] =
587642
values.flatMap(r => r.mechanicalFix.map(r.code -> _)).toMap
588643

589644
/** The subject prefix -- the part before the first `-`. Lets a consumer select a whole family. */
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/*
2+
* Copyright 2019-2026 Ossum Inc.
3+
*
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
package com.ossuminc.riddl.language
8+
9+
import com.ossuminc.riddl.utils.{AbstractTestingBasis, pc}
10+
11+
/** [1.18]: `pc.log` must return the SAME logger each call, on every platform.
12+
*
13+
* `DOMPlatformContext` overrode `def log` to return a fresh `SysLogger()` per call. That defeated
14+
* `withLogger` — the visible symptom — but it also silently zeroed the per-instance message
15+
* counters `Logger.summary` reports, because every `count()` landed on a different instance. The
16+
* second casualty was invisible until the first was diagnosed.
17+
*
18+
* SHARED on purpose: the defect was a platform difference, so a suite that skips the platform it
19+
* differs on cannot see it return.
20+
*/
21+
class LoggerIdentityTest extends AbstractTestingBasis {
22+
23+
"pc.log" should {
24+
25+
"return the same instance across calls, so state survives" in {
26+
// Reference identity is the whole assertion: a fresh logger per call is precisely the bug.
27+
assert(pc.log eq pc.log, "pc.log returned a different Logger on a second call")
28+
}
29+
30+
"accumulate message counts rather than losing them" in {
31+
val before = pc.log.summary
32+
pc.log.error("one")
33+
pc.log.error("two")
34+
val after = pc.log.summary
35+
// Counting into a discarded instance leaves the summary unchanged, which is what happened
36+
// on JS for as long as the override stood.
37+
assert(before != after, s"summary did not change after logging:\n$after")
38+
}
39+
}
40+
}

language/src/test/scala-jvm-native/com/ossuminc/riddl/language/RuleIdLogRenderingTest.scala renamed to language/src/test/scala/com/ossuminc/riddl/language/RuleIdLogRenderingTest.scala

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,15 @@ import com.ossuminc.riddl.utils.{AbstractTestingBasis, CallBackLogger, pc}
1010

1111
/** The rule id is rendered by the LOGGER, beside the kind prefix it already supplies.
1212
*
13-
* **This suite is `scala-jvm-native` because `withLogger` cannot capture on Scala.js.**
14-
* `DOMPlatformContext` overrides `def log` to return a fresh `SysLogger()` on every call
15-
* (`DOMPlatformContext.scala:88`), so the logger `withLogger` swaps into the `logger` field is
16-
* never consulted and a capture reads the empty string. The rendering itself is fine on JS -- CI
17-
* printed `[error] [field-duplicate-name] ...` in the very run where this assertion failed, which
18-
* is the tell that the INSTRUMENT was broken and not the feature. Filed as BACKLOG [1.18].
13+
* **This suite is SHARED again as of [1.18].** It lived in `scala-jvm-native` for a day because
14+
* `withLogger` could not capture on Scala.js: `DOMPlatformContext` overrode `def log` to return a
15+
* FRESH `SysLogger()` on every call, so the logger `withLogger` swapped into the `logger` field
16+
* was never consulted. The override also silently zeroed every per-instance message counter, and
17+
* it returned exactly what the base field is already initialised to -- so deleting it restored
18+
* `withLogger` and the counters while changing nothing about default behaviour.
19+
*
20+
* Running here on all three rows is the point: the defect was a PLATFORM difference, and a suite
21+
* that skips the platform it differs on cannot see it come back.
1922
*
2023
* The platform-independent half -- that the id stays OUT of `Message.format`, where
2124
* `CheckMessagesTest` compares its goldens -- is asserted in the shared `RuleIdTest`.

riddlLib/src/main/scala/com/ossuminc/riddl/RiddlLib.scala

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -971,8 +971,13 @@ object RiddlLib extends RiddlLib:
971971
// carries its own mechanical replacement cannot fall out of step with one.
972972
val edits = msgs.justDeprecations.toSeq.flatMap { m =>
973973
m.ruleId.flatMap(rule => rule.mechanicalFix.map(rule.code -> _)).map {
974-
case (code, replacement) =>
975-
SourceEdit(m.loc.offset, m.loc.endOffset, replacement, code, origin)
974+
case (code, fix) =>
975+
// A COMPUTED fix needs the text it matched, and this function has the source -- so it
976+
// slices the span rather than dropping the fix. `quoted-constant-literal` (`"5"` ->
977+
// `5`) is only expressible this way, which is why [1.16] made `Fix` a sum type
978+
// instead of widening the published `Map[String, String]` that cannot carry it.
979+
val matched = source.slice(m.loc.offset, m.loc.endOffset)
980+
SourceEdit(m.loc.offset, m.loc.endOffset, fix(matched), code, origin)
976981
}
977982
}
978983
// Descending, so applying them in order never shifts an offset still to be used.

utils/src/main/scalajs/com/ossuminc/riddl/utils/DOMPlatformContext.scala

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,6 @@ case class DOMPlatformContext() extends PlatformContext {
8585

8686
override def stderrln(message: String): Unit = dom.console.error(message + newline)
8787

88-
override def log: Logger = SysLogger()
89-
9088
override def newline: String = "\n"
9189

9290
override def ec: ExecutionContext = scala.concurrent.ExecutionContext.global

0 commit comments

Comments
 (0)