Make expression type checking actually check something - #182
Merged
Conversation
mdl/exprcheck ships a complete expression type checker whose semantic rules —
enum-value comparisons, attribute and operand type mismatches, function argument
types — all run through its CatalogReader seam. Nothing implemented that seam,
so every real invocation ran with a nil Catalog and every semantic rule was
silently skipped. The checker was in the tree and checked nothing; only the
narrow hand-rolled entry points (InferSourceKind, UnknownFunctionCalls) did any
work.
Three of the five lookups had no data behind them either, so this is two changes
in one place.
Catalog (schema 10, so cached catalogs regenerate — without the bump a stale
cache answers "unknown" for every lookup, which the checker reads as "cannot
tell" and skips, i.e. a green run that checked nothing):
- attributes_data.EnumerationQualifiedName. DataType comes from GetTypeName and
is the bare kind, so an enumeration attribute reported only "Enumeration" and
lost which enumeration. Kept as a new column rather than folded into DataType
as "Enumeration:QN", because existing queries and lint rules match DataType by
equality.
- enumeration_values_data. The table stored ValueCount but not the values, so
nothing could answer "is 'Open' a case of this enum" — the check behind the
most common expression bug.
- microflow_parameters_data, for microflows and nanoflows alike. Likewise
ParameterCount without the parameters. ParameterType reuses the ReturnType
encoding ("Object:Mod.Entity", "Enumeration:Mod.Enum", …).
mdl/exprcatalog implements all five methods over that data as an index loaded in
four queries, not SQL per lookup: a project has thousands of expressions each
asking several questions.
Two behaviours worth stating because they are choices, not accidents:
- Anything unanswerable returns (zero, false) → KindUnknown → the rule is
suppressed. A stale or partial catalog makes the checker catch less, never
false-positive on valid code.
- A Void return reports not-found rather than being mapped to KindEmpty.
Calling a void microflow in a value position is a real error, but inventing a
type for it here would diagnose it in the wrong place.
Known blind spot, verified rather than assumed: the System module contributes no
enumerations at all (`show enumerations in System` is empty on a stock 11.13
app) because they are platform metadata, not stored units. An attribute typed by
System.WorkflowEventType resolves its enum name but not its cases, so the
enum-value rule is skipped for it. Pre-existing, documented in the package.
Verified against a real 11.13.0 project, not only seeded rows: attribute kinds,
the attribute → enum → cases chain in model order, microflow and nanoflow return
kinds, and parameters by name with or without the $ sigil. Each new test was
shown to fail with the defect it pins (index handed out by reference, attributes
keyed without their entity, Void given a kind).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
With the CatalogReader seam implemented, the checker has answers but still no
caller: adapters.CheckAdapter was invoked by nothing but its own test. This
wires it to `mxcli check --references` via Executor.TypeCheckProgram, so the
catalog-backed rules actually report.
Codes stay exprcheck's own E0xx. Remapping to a TC0xx of our own would mean two
vocabularies for one diagnostic, and the code printed in the message would no
longer be the code you can look up in the hints registry.
Exercising it against a real project found two more defects in the ported
adapter, both of which had kept it silent regardless of the catalog:
- exprSource read only ast.SourceExpr. The visitor attaches one to some slots
and not others — on the fixture project neither a CREATE's nor a CHANGE's enum
value carried one — so the walk had nothing to parse. Now injectable via
WithSourceFunc, and the executor passes microflowExprSource, which falls back
to rendering the AST. Controls: with the adapter's default, 0 violations; with
the executor's, 2.
- Captured source arrives with the statement's trailing layout ("'Open'\n "),
now trimmed before parsing rather than left for the lexer to recover from.
Also adds CheckNanoflow: a nanoflow's body is the same []ast.MicroflowStatement
and every rule here is about expressions, so leaving nanoflows to reach for
CheckMicroflow would make the asymmetry look deliberate.
Placement: after the reference check, because a script naming things that do not
exist has a more basic problem than a mistyped operand, and building a catalog
for a run that already failed is wasted work. Fast-mode catalog is enough — none
of the type lookups need the full build.
Only an error severity fails the run, as everywhere else in this command. A
checker whose first outing turns advice into a broken build is a checker people
turn off.
False-positive probe before making errors non-zero-exit: all 21 microflows of a
Mendix 11.13.0 app (App, Administration, FeedbackModule, MyFirstModule — most of
them marketplace-authored and Studio Pro-validated) described back to MDL and
re-checked → 0 findings.
Known depth limit, stated rather than discovered later: inferKind returns
KindUnknown for AttributePathExpr, so `if $obj/Status = 'Open'` is still NOT
caught — only slot-qualified positions reach the catalog. Closing it needs the
var→entity scope, which exprcheck.Scope cannot carry (it speaks TypeKind only)
even though the adapter already computes the map. Recorded in the proposal as
the remaining Tier-2 item.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
One conflict, in cmd/mxcli/cmd_check.go's help text, and it is a semantic one rather than a textual clash: main (54c8f95) made --references implied by -p, because `mxcli check script.mdl -p app.mpr` used to print an unqualified "Check passed!" having resolved nothing. That is strictly better for this branch — expression type checking is inside the same block, so it now runs whenever a project is given rather than only behind an extra flag. Resolved by taking main's example line and rewriting this branch's paragraph, which said the type rules "run only with --references" and would have been wrong the moment it merged. The generated ANTLR parser needed regenerating again for main's queue clause (mdl/visitor references parser.IQueueClauseContext); `make build` does it. Re-verified after the merge, with the flag now omitted: the two-bug script still reports both E001s with -p alone, the corrected script still passes, and the false-positive probe over all 21 microflows of the 11.13.0 project is still clean. make test and make lint pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mdl/exprcheckis a complete expression type checker — parser, type lattice, function-return table, hints registry, slot resolver, and aContextcarrying the two tiersPROPOSAL_expression_type_checkingdescribes. It has been in the tree for a while and has never reported a single semantic finding on a real project.Three independent silent absences, each of which alone reduces it to a no-op:
exprcheck.CatalogReader.grepfor the interface outside its own package returned zero hits, so every invocation ran with a nilCatalog,IsSemanticEnabled()was false, and every semantic rule was skipped.adapters.CheckAdapter— the thing that walks a microflow and emits hints — was called by nothing but its own test.attributes_data.DataTypeis the bare kind ("Enumeration", losing which enumeration);enumerations_datastoredValueCountbut not the values;microflows_datastoredParameterCountbut not the parameters.exprSourceread onlyast.SourceExpr. The visitor attaches one to some slots and not others — on the fixture project neither a CREATE's nor a CHANGE's enum value carried one — so the walk had nothing to parse regardless of the catalog.This PR closes all three.
mxcli check --referencesnow reports:What changed
Catalog (schema 10) —
attributes_data.EnumerationQualifiedName,enumeration_values_data,microflow_parameters_data(microflows and nanoflows alike;ParameterTypereuses theReturnTypeencoding). The version bump matters more than usual: without it a cached catalog answers "unknown" for every lookup, which the checker reads as "cannot tell" and skips — the original bug wearing a fresh cache.EnumerationQualifiedNameis a new column rather than folding the enum intoDataTypeas"Enumeration:QN", because existing queries and lint rules matchDataTypeby equality.mdl/exprcatalog— implements all five methods as an index loaded in four queries, not SQL per lookup: a project has thousands of expressions each asking several questions.mdl/executor/typecheck.go—Executor.TypeCheckProgram, wired intomxcli check --references. Fast-mode catalog is enough; none of the type lookups need the full build.mdl/exprcheck/adapters—WithSourceFuncso a caller that can render the AST (the executor'sexpressionToString) supplies it; source trimmed before parsing;CheckNanoflowadded.Decisions worth reviewing
(zero, false)→KindUnknown→ rule suppressed. A stale or partial catalog makes the checker catch less, never false-positive on valid code. Correct for an advisory gate; explicitly not the reading a mutating consumer may inherit.Voidmicroflow return reports not-found rather than being mapped toKindEmpty. Calling a void flow in a value position is a real error, but inventing a type here would diagnose it in the wrong place.check. A checker whose first outing turns advice into a broken build is a checker people turn off.Verification
Against a real Mendix 11.13.0 project, not only seeded rows: attribute kinds, the attribute → enum → cases chain in model order, microflow and nanoflow return kinds, and parameters by name with or without the
$sigil.False-positive probe before letting errors exit non-zero: all 21 microflows of that app (App, Administration, FeedbackModule, MyFirstModule — most marketplace-authored and Studio Pro-validated) described back to MDL and re-checked → 0 findings.
Controls — every new test was shown to fail against the defect it pins:
Voidgiven a kindmake testandmake lintpass.Known limits, stated rather than discovered later
if $obj/Status = 'Open'is still NOT caught.inferKindreturnsKindUnknownforAttributePathExpr, so only slot-qualified positions (create/change members, where the adapter buildsCreateItem.Value:Entity.Attr) reach the catalog. Closing it needs the var→entity scope, whichexprcheck.Scopecannot carry — it speaksTypeKindonly — even though the adapter already computes the map and drops it. This is the proposal's remaining Tier-2 item, recorded there, and it is the same resolution the attribute rename's expression half needs.show enumerations in Systemis empty on a stock 11.13 app — they are platform metadata, not stored units), so an attribute typed by e.g.System.WorkflowEventTyperesolves its enum name but not its cases and the enum rule is skipped for it. Pre-existing; documented inmdl/exprcatalog.Noticed in passing, not touched
cmd/mxcli/cmd_rename.goissuesCONNECT LOCAL '…' FOR WRITING, but the grammar isCONNECT LOCAL STRING_LITERAL— there is noFOR WRITINGclause, and the parse errors are discarded (connectProg, _ := visitor.Build(...)), so the trailing tokens are silently dropped. Rename evidently works, so write mode comes from elsewhere, but that statement is not doing what it reads as. Worth a separate look.🤖 Generated with Claude Code
https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
Generated by Claude Code