diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 59cea5afc..807a52de7 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -65,6 +65,11 @@ run. Output includes structured rule IDs (MDL prefix for reference and script rules, E0xx for expression type rules) for each validation issue. +A deprecated MDL spelling — an alias left over from consolidating MDL onto one +canonical form, such as "create or replace" for "create or modify" or "show" for +"list" — is reported as an MDL-DEPRnnn warning naming the canonical form. Pass +--deprecations=error to fail the run on one instead, e.g. in CI over docs. + Use --post-migration to scan an existing project (independent of the script) for legacy native widgets that have pluggable replacements — Studio Pro does not auto-migrate these on a Mendix major-version upgrade. @@ -99,6 +104,7 @@ Examples: checkRefs, _ := cmd.Flags().GetBool("references") checkRefs = checkRefs || projectPath != "" postMigration, _ := cmd.Flags().GetBool("post-migration") + depPolicy := deprecationPolicy(cmd) format := resolveFormat(cmd, "text") isStructured := format != "" && format != "text" @@ -194,6 +200,7 @@ Examples: // refuses exactly what `mxcli check` reports. Adding a check there gives // both commands it at once. violations := append(testProblems, executor.ValidateProgram(prog, projectPath)...) + violations = executor.ApplyDeprecationPolicy(violations, depPolicy) if isStructured { // Always emit structured output (even when clean) diff --git a/cmd/mxcli/cmd_exec.go b/cmd/mxcli/cmd_exec.go index 317a57ba7..cea6849f4 100644 --- a/cmd/mxcli/cmd_exec.go +++ b/cmd/mxcli/cmd_exec.go @@ -25,6 +25,9 @@ applies statements one at a time and cannot roll back, so running a script with a known error leaves the model partly updated. Warnings are printed and do not stop the run. Use --no-check to apply a script anyway. +A deprecated MDL spelling (MDL-DEPRnnn, e.g. "create or replace" for "create or +modify") is a warning; --deprecations=error makes it an error. + By default execution stops at the first error. With --continue-on-error, every statement is attempted; each failure is reported (prefixed with its statement number) and execution continues, exiting non-zero if any statement failed. This @@ -50,6 +53,7 @@ Example: projectPath, _ := cmd.Flags().GetString("project") continueOnError, _ := cmd.Flags().GetBool("continue-on-error") skipCheck, _ := cmd.Flags().GetBool("no-check") + depPolicy := deprecationPolicy(cmd) // Read the script (a path, or "-" for stdin) content, err := readMDLSource(filePath) @@ -103,7 +107,7 @@ Example: // exec is not transactional, so "run it and see" means a half-applied // model. Warnings are printed and do not stop the run. if !skipCheck { - violations := executor.ValidateProgram(prog, projectPath) + violations := executor.ApplyDeprecationPolicy(executor.ValidateProgram(prog, projectPath), depPolicy) if len(violations) > 0 { formatter := linter.GetFormatter(linter.OutputFormatText, true) formatter.Format(violations, os.Stderr) diff --git a/cmd/mxcli/deprecations_flag.go b/cmd/mxcli/deprecations_flag.go new file mode 100644 index 000000000..db892c292 --- /dev/null +++ b/cmd/mxcli/deprecations_flag.go @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os" + + "github.com/mendixlabs/mxcli/mdl/deprecation" + "github.com/spf13/cobra" +) + +// The --deprecations flag decides what `check` and `exec` do with a deprecated +// MDL spelling (an MDL-DEPRnnn warning, registry in mdl/deprecation). The +// default warns; `error` fails the run, so CI over docs, skills and examples +// can hold them to the canonical form. +const deprecationsFlagUsage = "What to do with a deprecated MDL spelling (MDL-DEPRnnn): " + + "warn, or error to fail the run (for CI over docs, skills and examples)" + +func init() { + checkCmd.Flags().String("deprecations", "warn", deprecationsFlagUsage) + execCmd.Flags().String("deprecations", "warn", deprecationsFlagUsage) +} + +// deprecationPolicy reads --deprecations, exiting with a usage error on a value +// it does not know: silently treating a typo as `warn` would let a CI gate pass +// that was meant to fail. +func deprecationPolicy(cmd *cobra.Command) deprecation.Policy { + raw, _ := cmd.Flags().GetString("deprecations") + policy, err := deprecation.ParsePolicy(raw) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(2) + } + return policy +} diff --git a/docs-site/src/appendixes/error-messages.md b/docs-site/src/appendixes/error-messages.md index 6244249eb..c4dfbb4ac 100644 --- a/docs-site/src/appendixes/error-messages.md +++ b/docs-site/src/appendixes/error-messages.md @@ -142,6 +142,21 @@ iterator name exists, so mxbuild rejects this with CE0109 "Undefined variable The rule keys on **scope, not on the name**. `$item` is perfectly valid in a predicate when it is the enclosing loop's iterator, which is how the O(N) lookup idiom is written — inside `loop $item in $L`, `find($Others, Key = $item/Key)` navigates the loop's variable and is not flagged. +### MDL-DEPRnnn: Deprecated spelling + +``` +line 1: `create or replace …` (enumeration) is deprecated; write `create or modify …` +— same meaning. Refused from `mdl 2`. [MDL-DEPR001] +``` + +**Cause:** The script uses an alias that MDL is consolidating away (ADR-0010). The alias means exactly what the canonical form means, so the statement runs unchanged. The warning names the canonical form, and the language version whose header (`mdl ;`) will refuse the alias (ADR-0011). + +**Solution:** Write the canonical form the warning names. The rewrite is a mechanical keyword swap, and the suggestion line says which one. + +`check` and `exec` report these as **warnings**. To fail the run on one, for example in CI over documentation and examples, pass `--deprecations=error`. + +The registry of deprecated spellings is `mdl/deprecation/deprecation.go`. It holds the code, old form, canonical form, rewrite and removal version of each entry. A spelling is only registered where it means exactly the same as its canonical form. `create or replace view entity`, for example, drops and recreates the view entity, so it is not reported. Likewise `show` is reported only where its canonical form is `list` (plurals and relationship queries); `show entity X` or `show version` is not, because it becomes `describe` or a REPL command, not `list`. + ## mxcli Parser Errors diff --git a/mdl/ast/ast.go b/mdl/ast/ast.go index 3ad8bef9c..223a8ef2d 100644 --- a/mdl/ast/ast.go +++ b/mdl/ast/ast.go @@ -57,6 +57,11 @@ type Program struct { // next to knownActivityAnnotations, instead of being spread across the seven // visitor sites that read them. DocumentAnnotations []DocumentAnnotation + // Deprecations records every use of a deprecated spelling registered in + // mdl/deprecation, in source order. Both spellings build the same + // statements, so this is the only trace of which one the source used; it + // drives the MDL-DEPRnnn warnings and nothing else may branch on it. + Deprecations []DeprecatedSpelling // LanguageVersion is the MDL language version the script is written in: // the number in its `mdl ;` header, or mdl 0 when it has none @@ -71,6 +76,19 @@ type Program struct { LanguageNotes []LanguageNote } +// DeprecatedSpelling is one use of a deprecated spelling in the source. +type DeprecatedSpelling struct { + // Code is the registry code, MDL-DEPRnnn. + Code string + // Line and Column locate the deprecated token (1-based line, 0-based + // column, as ANTLR reports them). + Line int + Column int + // Subject says what the spelling was used on, in MDL's own words ("entity", + // "microflow", …); empty when there is nothing more specific to say. + Subject string +} + // LanguageNote is one construct whose meaning depends on the language version, // kept at the meaning of the version the script is written in. type LanguageNote struct { diff --git a/mdl/deprecation/deprecation.go b/mdl/deprecation/deprecation.go new file mode 100644 index 000000000..50d5c47eb --- /dev/null +++ b/mdl/deprecation/deprecation.go @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package deprecation is the single registry of deprecated MDL spellings +// (ADR-0011, decision 1). +// +// A deprecated spelling is a respelling: it means exactly what its canonical +// form means, so it keeps parsing, `check` and `exec` warn on it, and +// `mxcli fmt --upgrade` can rewrite it mechanically. A spelling that means +// something different from the proposed canonical form is NOT an alias there, +// and is not reported — rewriting it would silently change a script. +// +// # How an alias is marked +// +// Every grammar token or alternative that exists only as an alias carries a +// block comment naming its registry code, next to the alias itself: +// +// CREATE (OR (MODIFY | REPLACE /* @alias MDL-DEPR001 */))? +// +// ANTLR ignores the comment. TestGrammarAliasesAreRegistered (mdl/grammar) +// reads the .g4 sources and fails when a marker names a code with no entry +// here, or an entry is named by no marker. The marker is what makes a missing +// entry detectable: an alias is marked in the edit that adds it, and the marker +// cannot be satisfied without an entry. +// +// # How a use is detected +// +// Both spellings build the same AST, so the parse tree is the only place the +// source spelling is still visible. The visitor records every use of a +// registered spelling on ast.Program.Deprecations, and the executor's +// ValidateDeprecations turns the records into warnings (errors under +// --deprecations=error). This generalises MDL065, where the AST node itself +// carries the spelling flags. +package deprecation + +import ( + "fmt" + "strings" +) + +// Entry is one deprecated spelling. +type Entry struct { + // Code is the stable warning code, MDL-DEPRnnn. Never reused. + Code string + // Old is the deprecated form, as a reader would write it. + Old string + // Canonical is the form that replaces it (ADR-0010). + Canonical string + // Rewrite is the mechanical rewrite `fmt --upgrade` applies. + Rewrite Rewrite + // RemovedIn is the MDL language version (the `mdl ;` header) under which + // the old form is refused. Under earlier versions it warns (ADR-0011). + RemovedIn int + // Note is shown with the warning: scope limits, or where the canonical form + // is expected to move next. + Note string + // Example is a complete statement in the old form. A test parses it and + // requires exactly this code to be recorded. + Example string + // CanonicalExample is Example after Rewrite. A test requires it to record no + // deprecation and to build the same AST as Example — the proof that the + // rewrite does not change meaning. + CanonicalExample string +} + +// Rewrite replaces one keyword token of the deprecated form with another. It is +// the only shape the seeded entries need; a richer one is added with the first +// entry that needs it. +type Rewrite struct { + // Token is the keyword to replace, lower-case. + Token string + // Replacement is the keyword written in its place, lower-case. + Replacement string +} + +// Codes of the registered entries, for the visitor to record. +const ( + CreateOrReplace = "MDL-DEPR001" + Show = "MDL-DEPR002" +) + +// entries is the registry. Append only: a code is never reused or renumbered, +// because scripts, CI allowlists and docs refer to it. +var entries = []Entry{ + { + Code: CreateOrReplace, + Old: "create or replace …", + Canonical: "create or modify …", + Rewrite: Rewrite{Token: "replace", Replacement: "modify"}, + RemovedIn: 2, + Note: "Not reported where `or replace` means something else today: " + + "`create or replace view entity` (drops and recreates), " + + "`create or replace translations` (replaces the whole set), and " + + "`create or replace user role` / `demo user` (the `replace` is ignored).", + Example: "create or replace enumeration M.Color (Red 'Red');", + CanonicalExample: "create or modify enumeration M.Color (Red 'Red');", + }, + { + Code: Show, + Old: "show …", + Canonical: "list …", + Rewrite: Rewrite{Token: "show", Replacement: "list"}, + RemovedIn: 2, + Note: "Reported only for plurals and relationship queries, whose canonical " + + "form is `list`. Forms that name a single thing (`show entity X`, " + + "`show navigation`, `show project security`, …) become `describe`, and " + + "session state (`show version`, `show status`) a REPL command; they are " + + "not reported until those forms exist.", + Example: "show entities in M;", + CanonicalExample: "list entities in M;", + }, +} + +// All returns every registered entry, in code order. +func All() []Entry { + out := make([]Entry, len(entries)) + copy(out, entries) + return out +} + +// Lookup returns the entry for code. +func Lookup(code string) (Entry, bool) { + for _, e := range entries { + if e.Code == code { + return e, true + } + } + return Entry{}, false +} + +// IsDeprecationCode reports whether a violation rule ID is a registry code. +func IsDeprecationCode(ruleID string) bool { + return strings.HasPrefix(ruleID, "MDL-DEPR") +} + +// Policy is what `check` and `exec` do with a deprecated spelling. +type Policy int + +const ( + // Warn reports it and carries on. The default. + Warn Policy = iota + // Error fails the run, for CI over docs, skills and examples. + Error +) + +// ParsePolicy reads the --deprecations flag value. +func ParsePolicy(s string) (Policy, error) { + switch strings.ToLower(strings.TrimSpace(s)) { + case "", "warn": + return Warn, nil + case "error": + return Error, nil + } + return Warn, fmt.Errorf("invalid --deprecations value %q: want warn or error", s) +} diff --git a/mdl/deprecation/deprecation_test.go b/mdl/deprecation/deprecation_test.go new file mode 100644 index 000000000..8084f5c43 --- /dev/null +++ b/mdl/deprecation/deprecation_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: Apache-2.0 + +package deprecation + +import ( + "regexp" + "testing" +) + +// The registry is append-only and its codes are what scripts, CI allowlists +// and docs key on, so each entry must be complete and its code well-formed and +// unique. +func TestRegistryEntriesAreWellFormed(t *testing.T) { + code := regexp.MustCompile(`^MDL-DEPR\d{3}$`) + seen := map[string]bool{} + for _, e := range All() { + if !code.MatchString(e.Code) { + t.Errorf("code %q is not MDL-DEPRnnn", e.Code) + } + if seen[e.Code] { + t.Errorf("code %s registered twice", e.Code) + } + seen[e.Code] = true + if !IsDeprecationCode(e.Code) { + t.Errorf("IsDeprecationCode(%q) = false", e.Code) + } + if got, ok := Lookup(e.Code); !ok || got.Code != e.Code { + t.Errorf("Lookup(%q) = %v, %v", e.Code, got.Code, ok) + } + if e.Old == "" || e.Canonical == "" || e.Rewrite.Token == "" || e.Rewrite.Replacement == "" || + e.Example == "" || e.CanonicalExample == "" { + t.Errorf("%s is incomplete: %+v", e.Code, e) + } + // ADR-0011: an alias warns under the version that deprecates it (1) + // and is refused from a later one. + if e.RemovedIn < 2 { + t.Errorf("%s RemovedIn = %d, want >= 2", e.Code, e.RemovedIn) + } + } + if _, ok := Lookup("MDL-DEPR999"); ok { + t.Error("Lookup found an unregistered code") + } + if IsDeprecationCode("MDL065") { + t.Error("MDL065 treated as a registry code") + } +} + +// A typo in --deprecations must be an error, not a silent `warn`: that would +// let a CI gate pass that was meant to fail. +func TestParsePolicy(t *testing.T) { + for in, want := range map[string]Policy{"": Warn, "warn": Warn, "error": Error, " ERROR ": Error} { + got, err := ParsePolicy(in) + if err != nil || got != want { + t.Errorf("ParsePolicy(%q) = %v, %v; want %v", in, got, err, want) + } + } + for _, in := range []string{"errors", "fail", "warning", "bogus"} { + if _, err := ParsePolicy(in); err == nil { + t.Errorf("ParsePolicy(%q) accepted an unknown value", in) + } + } +} diff --git a/mdl/executor/validate_deprecations.go b/mdl/executor/validate_deprecations.go new file mode 100644 index 000000000..cd2723846 --- /dev/null +++ b/mdl/executor/validate_deprecations.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/deprecation" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ValidateDeprecations reports every use of a deprecated spelling the visitor +// recorded, one warning per use, under the entry's MDL-DEPRnnn code. +// +// A warning, never an error by itself: a deprecated spelling means exactly +// what its canonical form means, and scripts in the wild use it. `check` and +// `exec` promote these to errors only under --deprecations=error (see +// ApplyDeprecationPolicy), which is for CI over docs, skills and examples. +func ValidateDeprecations(prog *ast.Program) []linter.Violation { + var out []linter.Violation + for _, d := range prog.Deprecations { + e, ok := deprecation.Lookup(d.Code) + if !ok { + continue // the visitor records only registered codes; pinned by tests + } + on := "" + if d.Subject != "" { + on = " (" + d.Subject + ")" + } + out = append(out, linter.Violation{ + RuleID: e.Code, + Severity: linter.SeverityWarning, + Message: fmt.Sprintf("line %d: `%s`%s is deprecated; write `%s` — same meaning. "+ + "Refused from `mdl %d`.", d.Line, e.Old, on, e.Canonical, e.RemovedIn), + Suggestion: fmt.Sprintf("Replace `%s` with `%s`. %s", e.Rewrite.Token, e.Rewrite.Replacement, e.Note), + }) + } + return out +} + +// ApplyDeprecationPolicy returns violations with every MDL-DEPRnnn warning +// promoted to an error under deprecation.Error, and unchanged otherwise. +func ApplyDeprecationPolicy(violations []linter.Violation, policy deprecation.Policy) []linter.Violation { + if policy != deprecation.Error { + return violations + } + out := make([]linter.Violation, len(violations)) + for i, v := range violations { + if deprecation.IsDeprecationCode(v.RuleID) { + v.Severity = linter.SeverityError + } + out[i] = v + } + return out +} diff --git a/mdl/executor/validate_deprecations_test.go b/mdl/executor/validate_deprecations_test.go new file mode 100644 index 000000000..0a3244021 --- /dev/null +++ b/mdl/executor/validate_deprecations_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/deprecation" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func deprecationViolations(t *testing.T, src string, policy deprecation.Policy) []linter.Violation { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + var out []linter.Violation + for _, v := range ApplyDeprecationPolicy(ValidateProgram(prog, ""), policy) { + if deprecation.IsDeprecationCode(v.RuleID) { + out = append(out, v) + } + } + return out +} + +// `check` and `exec` share ValidateProgram, so a deprecated spelling warns in +// both. The canonical script is the control: it must stay silent, or the +// warning would fire on everything. +func TestDeprecatedSpellingsWarnThroughValidateProgram(t *testing.T) { + old := "create or replace enumeration M.Color (Red 'Red');\nshow entities in M;\n" + canon := "create or modify enumeration M.Color (Red 'Red');\nlist entities in M;\n" + + got := deprecationViolations(t, old, deprecation.Warn) + if len(got) != 2 || got[0].RuleID != deprecation.CreateOrReplace || got[1].RuleID != deprecation.Show { + t.Fatalf("got %+v, want MDL-DEPR001 then MDL-DEPR002", got) + } + for _, v := range got { + if v.Severity != linter.SeverityWarning { + t.Errorf("%s severity = %v, want warning — an error would refuse working scripts", v.RuleID, v.Severity) + } + } + if !strings.Contains(got[0].Message, "line 1") || !strings.Contains(got[0].Message, "create or modify") { + t.Errorf("message must name the line and the canonical form: %q", got[0].Message) + } + if !strings.Contains(got[1].Message, "line 2") || !strings.Contains(got[1].Message, "list") { + t.Errorf("message must name the line and the canonical form: %q", got[1].Message) + } + if summary := linter.Summarize(ValidateProgram(mustProgram(t, old), "")); summary.Errors > 0 { + t.Errorf("deprecated spellings produced %d error(s) by default; exec would refuse the script", summary.Errors) + } + + if got := deprecationViolations(t, canon, deprecation.Warn); len(got) != 0 { + t.Errorf("canonical script reported %+v", got) + } +} + +func TestDeprecationPolicyErrorFailsTheRun(t *testing.T) { + src := "show modules;" + got := deprecationViolations(t, src, deprecation.Error) + if len(got) != 1 || got[0].Severity != linter.SeverityError { + t.Fatalf("under --deprecations=error got %+v, want one error", got) + } + // Only registry codes are promoted: another warning stays a warning. + other := []linter.Violation{{RuleID: "MDL065", Severity: linter.SeverityWarning}} + if v := ApplyDeprecationPolicy(other, deprecation.Error); v[0].Severity != linter.SeverityWarning { + t.Errorf("MDL065 promoted to %v; only MDL-DEPRnnn codes may be", v[0].Severity) + } +} + +func mustProgram(t *testing.T, src string) *ast.Program { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + return prog +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index d06a34470..c83f20338 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -295,6 +295,11 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // parsed and did nothing (MDL059, the same rule statements already have). violations = append(violations, ValidateDocumentAnnotations(prog)...) + // Warn on every deprecated spelling (MDL-DEPRnnn): an alias left over from + // consolidating MDL onto one canonical form (ADR-0010/0011). The registry + // is mdl/deprecation. + violations = append(violations, ValidateDeprecations(prog)...) + // The `mdl ;` header: a preview version warns that it may still change, // and every construct kept at an older meaning warns (ADR-0011). violations = append(violations, ValidateLanguageVersion(prog)...) diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 225eb0e35..7c4d53bcf 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -107,7 +107,7 @@ updateWidgetsStatement createStatement : docComment? annotation* - CREATE (OR (MODIFY | REPLACE))? + CREATE (OR (MODIFY | REPLACE /* @alias MDL-DEPR001 */))? ( createEntityStatement | createAssociationStatement | createModuleStatement diff --git a/mdl/grammar/alias_registry_test.go b/mdl/grammar/alias_registry_test.go new file mode 100644 index 000000000..1a240bf5e --- /dev/null +++ b/mdl/grammar/alias_registry_test.go @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: Apache-2.0 + +package grammar + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/deprecation" +) + +// aliasMarker is how a grammar token or alternative is marked as a deprecated +// alias: a block comment naming its registry code, e.g. +// +// CREATE (OR (MODIFY | REPLACE /* @alias MDL-DEPR001 */))? +// +// ANTLR drops the comment; this test is its only reader. +var aliasMarker = regexp.MustCompile(`@alias\b\s*(\S*)`) + +var deprCode = regexp.MustCompile(`^MDL-DEPR\d{3}$`) + +// checkAliasMarkers returns one problem per marker that is malformed or names a +// code with no registry entry, and per registry entry that no marker names. +// sources maps a file name to its grammar text. +func checkAliasMarkers(sources map[string]string, entries []deprecation.Entry) []string { + registered := map[string]bool{} + for _, e := range entries { + registered[e.Code] = true + } + marked := map[string]bool{} + var problems []string + + names := make([]string, 0, len(sources)) + for n := range sources { + names = append(names, n) + } + sort.Strings(names) + for _, name := range names { + for i, line := range strings.Split(sources[name], "\n") { + for _, m := range aliasMarker.FindAllStringSubmatch(line, -1) { + code := strings.TrimSuffix(m[1], "*/") + switch { + case !deprCode.MatchString(code): + problems = append(problems, fmt.Sprintf( + "%s:%d: malformed alias marker %q — write /* @alias MDL-DEPRnnn */", name, i+1, m[0])) + case !registered[code]: + problems = append(problems, fmt.Sprintf( + "%s:%d: grammar alias %s has no entry in mdl/deprecation — add one "+ + "(old form, canonical form, rewrite, removed-in version, examples)", name, i+1, code)) + default: + marked[code] = true + } + } + } + } + for _, e := range entries { + if !marked[e.Code] { + problems = append(problems, fmt.Sprintf( + "registry entry %s (%s) is named by no /* @alias %s */ marker in the grammar — "+ + "mark the alias where it is parsed", e.Code, e.Old, e.Code)) + } + } + return problems +} + +func readGrammarSources(t *testing.T) map[string]string { + t.Helper() + files, err := filepath.Glob("*.g4") + if err != nil { + t.Fatal(err) + } + domain, err := filepath.Glob("domains/*.g4") + if err != nil { + t.Fatal(err) + } + files = append(files, domain...) + if len(files) == 0 { + t.Fatal("no .g4 files found") + } + sources := map[string]string{} + for _, f := range files { + b, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + sources[f] = string(b) + } + return sources +} + +// An alias in the grammar must have a registry entry, so that it warns and +// `fmt --upgrade` can rewrite it (ADR-0011, decision 1) — and an entry must +// point at a real alias. +func TestGrammarAliasesAreRegistered(t *testing.T) { + for _, p := range checkAliasMarkers(readGrammarSources(t), deprecation.All()) { + t.Error(p) + } +} + +// The control: the checker does fail on an alias with no entry, on an entry +// with no alias, and on a malformed marker. Without this, a checker that found +// nothing would pass the test above against any grammar. +func TestAliasMarkerCheckerDetectsProblems(t *testing.T) { + entries := []deprecation.Entry{{Code: "MDL-DEPR001", Old: "a"}, {Code: "MDL-DEPR002", Old: "b"}} + grammar := map[string]string{"X.g4": strings.Join([]string{ + "r1 : A /* @alias MDL-DEPR001 */ | B ;", + "r2 : C /* @alias MDL-DEPR999 */ | D ;", + "r3 : E /* @alias DEPR3 */ ;", + }, "\n")} + problems := checkAliasMarkers(grammar, entries) + want := []string{"MDL-DEPR999 has no entry", "malformed alias marker", "MDL-DEPR002 (b) is named by no"} + for _, w := range want { + found := false + for _, p := range problems { + if strings.Contains(p, w) { + found = true + } + } + if !found { + t.Errorf("checker did not report %q; got %q", w, problems) + } + } + if len(problems) != len(want) { + t.Errorf("got %d problems, want %d: %q", len(problems), len(want), problems) + } +} diff --git a/mdl/grammar/domains/MDLCatalog.g4 b/mdl/grammar/domains/MDLCatalog.g4 index d5e1a0bf4..97cc259b0 100644 --- a/mdl/grammar/domains/MDLCatalog.g4 +++ b/mdl/grammar/domains/MDLCatalog.g4 @@ -9,7 +9,7 @@ options { tokenVocab = MDLLexer; } // DQL STATEMENTS (Data Query Language) // ============================================================================= -showOrList: SHOW | LIST_KW ; +showOrList: SHOW /* @alias MDL-DEPR002 */ | LIST_KW ; showStatement : showOrList MODULES diff --git a/mdl/visitor/visitor.go b/mdl/visitor/visitor.go index 410342d16..c4c4f6bb7 100644 --- a/mdl/visitor/visitor.go +++ b/mdl/visitor/visitor.go @@ -477,6 +477,9 @@ type Builder struct { // documentAnnotations collects every `@name` written before a CREATE, with // the kind of document it was on — see ExitCreateStatement. documentAnnotations []ast.DocumentAnnotation + // deprecations collects every use of a deprecated spelling — see + // visitor_deprecations.go. + deprecations []ast.DeprecatedSpelling // inLayout is set while a CREATE LAYOUT body is being built. The page body // builder serves both documents and cannot otherwise tell which it is in, @@ -560,6 +563,7 @@ func build(input string, listen func(*Builder) antlr.ParseTreeListener) (*ast.Pr return &ast.Program{ Statements: builder.statements, DocumentAnnotations: builder.documentAnnotations, + Deprecations: builder.deprecations, LanguageVersion: builder.langVersion, LanguageHeaderLine: builder.langHeaderLine, LanguageNotes: builder.langNotes, diff --git a/mdl/visitor/visitor_deprecations.go b/mdl/visitor/visitor_deprecations.go new file mode 100644 index 000000000..72e44bc22 --- /dev/null +++ b/mdl/visitor/visitor_deprecations.go @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/deprecation" + "github.com/mendixlabs/mxcli/mdl/grammar/parser" + + "github.com/antlr4-go/antlr/v4" +) + +// Deprecated spellings are recorded from the parse tree because it is the only +// place they are visible: `create or replace` and `create or modify`, `show` +// and `list`, build the same statements. The registry of spellings, and how a +// grammar alias is marked, is in mdl/deprecation. + +// recordDeprecation appends one use of a deprecated spelling at tok. +func (b *Builder) recordDeprecation(code string, tok antlr.Token, subject string) { + if tok == nil { + return + } + b.deprecations = append(b.deprecations, ast.DeprecatedSpelling{ + Code: code, + Line: tok.GetLine(), + Column: tok.GetColumn(), + Subject: subject, + }) +} + +// createOrReplaceIsNotAnAlias lists the create kinds (createStatementKind's +// words) where `or replace` does NOT mean `or modify`, so rewriting it would +// change the script. Measured against the visitors, and pinned by +// TestCreateOrReplaceMatchesModifyExceptExemptKinds: +// +// - translations: `or replace` replaces the whole set, `or modify` merges. +// - userrole, demouser: the visitor reads only `or modify`; `or replace` is +// a plain create, which fails on an existing role or user. +// +// `create or replace view entity` (kind "entity") drops and recreates the view +// entity, and is exempted in recordCreateOrReplace. +var createOrReplaceIsNotAnAlias = map[string]bool{ + "translations": true, + "userrole": true, + "demouser": true, +} + +// recordCreateOrReplace records MDL-DEPR001 for a `create or replace` whose +// meaning is exactly `create or modify`. +func (b *Builder) recordCreateOrReplace(ctx *parser.CreateStatementContext) { + if ctx.OR() == nil || ctx.REPLACE() == nil { + return + } + kind := createStatementKind(ctx) + if kind == "" || createOrReplaceIsNotAnAlias[kind] { + return + } + if ent, ok := ctx.CreateEntityStatement().(*parser.CreateEntityStatementContext); ok && ent.VIEW() != nil { + return + } + b.recordDeprecation(deprecation.CreateOrReplace, ctx.REPLACE().GetSymbol(), kind) +} + +// showNotYetList lists the showStatement forms whose decided canonical form is +// NOT `list` (PROPOSAL_mdl_beta_syntax_freeze.md §3, R6): a single thing +// becomes `describe`, session state a REPL command. Keyed on the token after +// `show` (CATALOG only with STATUS, see showCanonicalIsList). `list` builds the +// same statement for these today, but it is not their canonical form, so +// recommending it would name the wrong form and make `fmt --upgrade` rewrite +// them twice. They get their own registry entries once the canonical forms +// exist (plan item 3.5). Pinned by TestShowRecordsDeprecation. +var showNotYetList = map[int]bool{ + parser.MDLParserENTITY: true, // show entity X -> describe entity X + parser.MDLParserASSOCIATION: true, // show association X -> describe association X + parser.MDLParserPAGE: true, // show page X -> describe page X + parser.MDLParserNAVIGATION: true, // show navigation … -> describe navigation + parser.MDLParserSTRUCTURE: true, // show structure -> describe structure + parser.MDLParserCONTEXT: true, // show context of X -> describe context of X + parser.MDLParserPROJECT: true, // show project security -> describe app security + parser.MDLParserSECURITY: true, // show security matrix -> describe security matrix + parser.MDLParserSETTINGS: true, // show settings -> describe (one thing) + parser.MDLParserVERSION: true, // session state -> REPL command (R7) + parser.MDLParserSTATUS: true, // session state -> REPL command (R7) + parser.MDLParserCONNECTIONS: true, // session state -> REPL command (R7) +} + +// showCanonicalIsList reports whether the showStatement that ctx starts is a +// form whose canonical spelling is `list` (plurals and relationship queries). +func showCanonicalIsList(ctx *parser.ShowOrListContext) bool { + stmt, ok := ctx.GetParent().(*parser.ShowStatementContext) + if !ok || stmt.GetChildCount() < 2 { + return true + } + next := func(i int) int { + if i >= stmt.GetChildCount() { + return antlr.TokenInvalidType + } + if tn, ok := stmt.GetChild(i).(antlr.TerminalNode); ok { + return tn.GetSymbol().GetTokenType() + } + return antlr.TokenInvalidType + } + first := next(1) + if first == parser.MDLParserCATALOG { + return next(2) != parser.MDLParserSTATUS // catalog status is session state + } + return !showNotYetList[first] +} + +// ExitShowOrList records MDL-DEPR002 for `show` where its canonical form is +// `list`. showOrList is used only by showStatement, where `show` and `list` +// build the same statement. +func (b *Builder) ExitShowOrList(ctx *parser.ShowOrListContext) { + if ctx == nil || ctx.SHOW() == nil || !showCanonicalIsList(ctx) { + return + } + b.recordDeprecation(deprecation.Show, ctx.SHOW().GetSymbol(), "") +} diff --git a/mdl/visitor/visitor_deprecations_test.go b/mdl/visitor/visitor_deprecations_test.go new file mode 100644 index 000000000..6bb425c07 --- /dev/null +++ b/mdl/visitor/visitor_deprecations_test.go @@ -0,0 +1,272 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "os" + "reflect" + "regexp" + "sort" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/deprecation" +) + +func deprecationCodes(prog *ast.Program) []string { + var out []string + for _, d := range prog.Deprecations { + out = append(out, d.Code) + } + return out +} + +func mustBuild(t *testing.T, src string) *ast.Program { + t.Helper() + prog, errs := Build(src) + if len(errs) > 0 { + t.Fatalf("parse %q: %v", src, errs) + } + return prog +} + +// Every registry entry's own example must be detected, and its canonical +// rewrite must be silent AND build the same statements. The last part is what +// makes the rewrite safe to apply mechanically (ADR-0011: a wrong rewrite is a +// silent change of meaning). +func TestRegistryExamplesRecordTheirCode(t *testing.T) { + for _, e := range deprecation.All() { + t.Run(e.Code, func(t *testing.T) { + old := mustBuild(t, e.Example) + if got := deprecationCodes(old); !reflect.DeepEqual(got, []string{e.Code}) { + t.Errorf("Example %q recorded %v, want [%s]", e.Example, got, e.Code) + } + canon := mustBuild(t, e.CanonicalExample) + if got := deprecationCodes(canon); len(got) != 0 { + t.Errorf("CanonicalExample %q recorded %v, want none", e.CanonicalExample, got) + } + if !reflect.DeepEqual(old.Statements, canon.Statements) { + t.Errorf("Example and CanonicalExample build different statements:\n old: %#v\n canon: %#v", + old.Statements, canon.Statements) + } + // The rewrite is a token swap; the canonical example must be exactly it. + re := regexp.MustCompile(`(?i)\b` + regexp.QuoteMeta(e.Rewrite.Token) + `\b`) + if got := re.ReplaceAllString(e.Example, e.Rewrite.Replacement); got != e.CanonicalExample { + t.Errorf("Rewrite %s -> %s gives %q, CanonicalExample is %q", + e.Rewrite.Token, e.Rewrite.Replacement, got, e.CanonicalExample) + } + }) + } +} + +// createOrReplaceCases holds one statement body per document kind that +// createStatement accepts. TestCreateOrReplaceMatchesModifyExceptExemptKinds +// builds each with `create or replace` and with `create or modify`. +var createOrReplaceCases = map[string]string{ + "entity": "persistent entity M.Customer (Name: String(200));", + "association": "association M.Order_Customer from M.Order to M.Customer type Reference;", + "module": "module M;", + "microflow": "microflow M.ACT_Recalculate () begin return; end;", + "javaaction": "java action M.FormatCurrency(Amount: Decimal not null) returns String as $$return \"\";$$;", + "javascriptaction": "javascript action M.IsStrictMode() returns Boolean platform Web as $$return true;$$;", + "page": "page M.OrderList (Title: 'Orders', Layout: Atlas_Core.Atlas_Default) { dynamictext txtHeading (Content: 'Orders') };", + "layout": "layout M.App_Default (layouttype: 'Responsive') { placeholder Main }", + "snippet": "snippet M.CustomerInfo { dynamictext t (Content: 'x') }", + "enumeration": "enumeration M.Color (Red 'Red');", + "validationrule": "validation rule for M.Customer.Email regex M.EmailPattern feedback 'Invalid';", + "databaseconnection": "database connection M.Erp type 'PostgreSQL' connection string @M.DbUrl username @M.DbUser password @M.DbPass;", + "constant": "constant M.ApiBaseUrl type String default 'https://api.example.com';", + "restclient": "rest client M.PetStore (BaseUrl: 'https://petstore.example.com', Authentication: NONE) { };", + "index": "index idx_name on M.Customer (Name);", + "odataclient": "odata client M.Api (Version: '1.0', ODataVersion: OData4, MetadataUrl: 'https://api.example.com/$metadata');", + "odataservice": "odata service M.CustomerAPI (path: 'odata/customers/', version: '1.0.0', ODataVersion: OData4, namespace: 'M.Customers') authentication basic { };", + "externalentity": "external entity M.Remote from odata client M.Api (EntitySet: 'Remotes', RemoteName: 'Remote');", + "externalentities": "external entities from M.Api into Integration;", + "navigation": "navigation Responsive home page M.Home_Web;", + "businesseventservice": "business event service M.CustomerEventsApi (ServiceName: 'CustomerEventsApi', EventNamePrefix: '') { message CustomerChangedEvent (CustomerId: Long) publish entity M.PBE_CustomerChangedEvent; };", + "workflow": "workflow M.LeaveApproval parameter $Context: M.LeaveRequest begin end workflow;", + "userrole": "user role Clerk (M.User);", + "demouser": "demo user 'demo' password 'Password1!' (Clerk);", + "imagecollection": "image collection M.AppIcons;", + "annotation": "annotation in M (Caption: 'Orders', Position: (60, 40));", + "queue": "queue M.Q_Orders (Parallelism: 3);", + "scheduledevent": "scheduled event M.NightlyCleanup (Microflow: M.SE_Cleanup, Repeat: Daily, HourOfDay: 4, MinuteOfHour: 0, TimeZone: Server, Enabled: true);", + "regularexpression": "regular expression M.Email (Expression: '.+@.+');", + "jsonstructure": "json structure M.JSON_Pet snippet '{\"id\": 1}';", + "messagedefinitioncollection": "message definition collection M.MD_Order (definition OrderMessage for M.Order as 'Orders' (OrderId));", + "importmapping": "import mapping M.IMM_Order with json structure M.JSON_Order { create M.Order { Id = id } };", + "exportmapping": "export mapping M.EMM_Order with json structure M.JSON_Order { M.Order { orderId = OrderId } };", + "configuration": "configuration 'Default';", + "publishedrestservice": "published rest service M.OrderAPI (Path: 'rest/orders/v1', Version: '1.0.0', ServiceName: 'Order API') { };", + "datatransformer": "data transformer M.Flatten source json '{\"id\": 1}' { jslt '{\"id\": .id}'; };", + "model": "model M.GPT4 (Provider: MxCloudGenAI, Key: M.ModelApiKey);", + "consumedmcpservice": "consumed mcp service M.WebSearch (ProtocolVersion: v2025_03_26, Version: '1.0');", + "knowledgebase": "knowledge base M.Docs (Provider: MxCloudGenAI, Key: M.KBApiKey);", + "agent": "agent M.Summarizer (UsageType: Task, Model: M.GPT4, SystemPrompt: 'Summarize.', UserPrompt: 'Text.');", + "nanoflow": "nanoflow M.NF_Validate () begin return; end;", + "rule": "rule M.Rule_IsSolvent ($c: M.Customer) returns Boolean begin return true; end;", + "menu": "menu M.Main_Menu (menu item 'Plain';);", + "translations": "translations in Administration for nl_NL ('Save' as 'Opslaan');", +} + +// foldPageModeFlags erases the one AST difference between `or replace` and +// `or modify` that means nothing: pages, snippets and layouts keep both flags, +// and every reader in the executor tests `IsModify || IsReplace`. +func foldPageModeFlags(stmts []ast.Statement) { + for _, s := range stmts { + switch n := s.(type) { + case *ast.CreatePageStmtV3: + n.IsModify, n.IsReplace = n.IsModify || n.IsReplace, false + case *ast.CreateSnippetStmtV3: + n.IsModify, n.IsReplace = n.IsModify || n.IsReplace, false + case *ast.CreateLayoutStmt: + n.IsModify, n.IsReplace = n.IsModify || n.IsReplace, false + } + } +} + +// createStatementKinds reads the document kinds createStatement accepts from +// the grammar source, so a kind added there fails this test until it has a case. +func createStatementKinds(t *testing.T) []string { + t.Helper() + src, err := os.ReadFile("../grammar/MDLParser.g4") + if err != nil { + t.Fatal(err) + } + s := string(src) + start := strings.Index(s, "\ncreateStatement\n") + if start < 0 { + t.Fatal("createStatement rule not found in MDLParser.g4") + } + end := strings.Index(s[start:], "\n ;") + body := s[start : start+end] + var kinds []string + for _, m := range regexp.MustCompile(`\bcreate(\w+)Statement\b`).FindAllStringSubmatch(body, -1) { + kinds = append(kinds, strings.ToLower(m[1])) + } + sort.Strings(kinds) + return kinds +} + +// `create or replace` is reported as MDL-DEPR001 exactly where it means +// `create or modify`: the two build the same statements. Where they do not, +// it is not an alias, and reporting it would tell the user to make a change +// that alters their script. Both directions are asserted, so the exemption list +// in the visitor cannot drift from what the visitors actually do. +func TestCreateOrReplaceMatchesModifyExceptExemptKinds(t *testing.T) { + for _, kind := range createStatementKinds(t) { + if _, ok := createOrReplaceCases[kind]; !ok { + t.Errorf("create kind %q has no case in createOrReplaceCases", kind) + } + } + + cases := make(map[string]string, len(createOrReplaceCases)+1) + for k, v := range createOrReplaceCases { + cases[k] = v + } + // A view entity is kind "entity" too, and its `or replace` drops and recreates. + cases["entity (view)"] = "view entity M.V (Name: String(100)) as (select c.Name as Name from M.Customer as c);" + + for name, body := range cases { + t.Run(name, func(t *testing.T) { + rep := mustBuild(t, "create or replace "+body) + mod := mustBuild(t, "create or modify "+body) + if len(rep.Statements) != 1 || len(mod.Statements) != 1 { + t.Fatalf("want one statement each, got %d and %d — the case does not exercise the visitor", + len(rep.Statements), len(mod.Statements)) + } + foldPageModeFlags(rep.Statements) + foldPageModeFlags(mod.Statements) + same := reflect.DeepEqual(rep.Statements, mod.Statements) + exempt := createOrReplaceIsNotAnAlias[name] || name == "entity (view)" + + switch { + case exempt && same: + t.Errorf("exempt, but `or replace` and `or modify` build the same statements: " + + "it is an alias after all — remove the exemption") + case !exempt && !same: + t.Errorf("`or replace` and `or modify` build different statements, so rewriting one "+ + "to the other changes the script — exempt this kind or fix the visitor:\n"+ + " replace: %#v\n modify: %#v", rep.Statements, mod.Statements) + } + + got := deprecationCodes(rep) + if exempt { + if len(got) != 0 { + t.Errorf("exempt kind recorded %v", got) + } + return + } + if !reflect.DeepEqual(got, []string{deprecation.CreateOrReplace}) { + t.Errorf("recorded %v, want [%s]", got, deprecation.CreateOrReplace) + } else if rep.Deprecations[0].Subject != name { + t.Errorf("subject = %q, want %q", rep.Deprecations[0].Subject, name) + } + if got := deprecationCodes(mod); len(got) != 0 { + t.Errorf("`create or modify` recorded %v", got) + } + }) + } +} + +func TestShowRecordsDeprecation(t *testing.T) { + cases := []struct { + src string + want []string + }{ + {"show entities;", []string{deprecation.Show}}, + {"SHOW MICROFLOWS IN M;", []string{deprecation.Show}}, + {"show callers of M.MF transitive;", []string{deprecation.Show}}, + {"show catalog tables;", []string{deprecation.Show}}, + {"show access on M.E;", []string{deprecation.Show}}, + {"show design properties;", []string{deprecation.Show}}, + {"show widgets;", []string{deprecation.Show}}, + // Not reported: the decided canonical form of these is NOT `list` + // (PROPOSAL_mdl_beta_syntax_freeze.md §3, R6). Single things become + // `describe`, session state a REPL command, so `list entity M.E` or + // `list version` would name a non-canonical form and make + // `fmt --upgrade` rewrite them twice. + {"show entity M.E;", nil}, + {"show association M.A;", nil}, + {"show page M.P;", nil}, + {"show navigation;", nil}, + {"show navigation homes;", nil}, + {"show navigation menu M.Nav;", nil}, + {"show structure depth 2 in M;", nil}, + {"show context of M.MF;", nil}, + {"show project security;", nil}, + {"show security matrix in M;", nil}, + {"show settings;", nil}, + {"show version;", nil}, + {"show status;", nil}, + {"show connections;", nil}, + {"show catalog status;", nil}, + {"list entities;", nil}, + {"describe entity M.E;", nil}, + // Not the statement keyword: microflow activities spelled `show`. + {"create or modify microflow M.MF () begin show page M.P(); show message 'hi' type Information; end;", nil}, + // `show lint rules` has no `list` form yet, so it is not an alias. + {"show lint rules;", nil}, + } + for _, c := range cases { + t.Run(c.src, func(t *testing.T) { + if got := deprecationCodes(mustBuild(t, c.src)); !reflect.DeepEqual(got, c.want) { + t.Errorf("recorded %v, want %v", got, c.want) + } + }) + } +} + +// The record points at the deprecated token, so a warning in a long script +// names the right line. +func TestDeprecationRecordsPosition(t *testing.T) { + prog := mustBuild(t, "list modules;\n\n create or replace enumeration M.C (A 'A');\nshow modules;") + want := []ast.DeprecatedSpelling{ + {Code: deprecation.CreateOrReplace, Line: 3, Column: 12, Subject: "enumeration"}, + {Code: deprecation.Show, Line: 4, Column: 0}, + } + if !reflect.DeepEqual(prog.Deprecations, want) { + t.Errorf("got %+v\nwant %+v", prog.Deprecations, want) + } +} diff --git a/mdl/visitor/visitor_document_annotations.go b/mdl/visitor/visitor_document_annotations.go index 91fcf805c..a1b499ef2 100644 --- a/mdl/visitor/visitor_document_annotations.go +++ b/mdl/visitor/visitor_document_annotations.go @@ -28,6 +28,7 @@ func (b *Builder) ExitCreateStatement(ctx *parser.CreateStatementContext) { if ctx == nil { return } + b.recordCreateOrReplace(ctx) anns := ctx.AllAnnotation() if len(anns) == 0 { return