diff --git a/changes/unreleased/record-sequence-compat.fixed.md b/changes/unreleased/record-sequence-compat.fixed.md new file mode 100644 index 0000000000..ecf6975a85 --- /dev/null +++ b/changes/unreleased/record-sequence-compat.fixed.md @@ -0,0 +1,2 @@ +- **A sequence mixing a quantity with a plain number records as text.** It has no single unit to record under, so it no longer settles to a bare Real that drops the measured element's unit. +- **A repeated sequence value is refused by an existing unique record member.** Recording into a definition that declares the member `[0..*]` without `nonunique` names the `into` remedy instead of generating a record that fails validation; generated definitions, declared `ordered nonunique`, keep admitting repeats. diff --git a/internal/exec/analysis/record/record.go b/internal/exec/analysis/record/record.go index dea250b7de..80fa833510 100644 --- a/internal/exec/analysis/record/record.go +++ b/internal/exec/analysis/record/record.go @@ -112,6 +112,9 @@ type Feature struct { // Multi marks a feature whose declared multiplicity admits more than one // value. Multi bool + + // Unique marks a multi-valued feature that holds no two equal values. + Unique bool } // Existing is what Generate must fit the records it makes into. @@ -180,11 +183,12 @@ var reservedFeatures = map[string]bool{ // feature is one member the record definition declares for a run value. type feature struct { - name string - ref bool // object-valued - typ string // declared type as written, "" for a ref - unitOf string // nonempty: this feature is the unit companion of the named one - multi bool // declares [0..*] + name string + ref bool // object-valued + typ string // declared type as written, "" for a ref + unitOf string // nonempty: this feature is the unit companion of the named one + multi bool // declares [0..*] + repeated bool // a sequence contains equal elements } // valueKind classifies how a value is spelled: its declared type and, for a @@ -214,11 +218,12 @@ const ( // whether the member is multi-valued, and — for a quantity — the unit text its // companion feature records. type shape struct { - kind valueKind - literal string - typ string - unit string - multi bool + kind valueKind + literal string + typ string + unit string + multi bool + repeated bool } // classify decides the feature shape a value asks for. @@ -269,7 +274,6 @@ func classify(v runtime.Value, r *Run) shape { // beside its kind, so the whole value falls back to its text. The empty sequence // is unset but multi-valued: it settles a member to [0..*] and spells `()`. func classifySequence(v runtime.Value, r *Run) shape { - fallback := shape{kind: kindString, typ: scalarValuesString, literal: source.StringText(spellText(v, r))} seq := v.Sequence() var elements []runtime.Value if seq != nil { @@ -278,6 +282,8 @@ func classifySequence(v runtime.Value, r *Run) shape { if len(elements) == 0 { return shape{kind: kindUnset, multi: true, literal: "()"} } + repeated := hasRepeatedElement(elements) + fallback := shape{kind: kindString, typ: scalarValuesString, literal: source.StringText(spellText(v, r)), repeated: repeated} literals := make([]string, 0, len(elements)) var settled shape for i, element := range elements { @@ -291,7 +297,9 @@ func classifySequence(v runtime.Value, r *Run) shape { case es.kind == settled.kind && es.unit == settled.unit && (es.kind != kindEnum || es.typ == settled.typ): // Same kind; for a quantity es.unit == settled.unit holds the share, and // enumeration literals must spell literals of the one enum. - case numericPair(es.typ, settled.typ): + case (es.kind == kindInteger || es.kind == kindReal) && + (settled.kind == kindInteger || settled.kind == kindReal) && + numericPair(es.typ, settled.typ): settled = shape{kind: kindReal, typ: scalarValuesReal} default: return fallback @@ -299,6 +307,7 @@ func classifySequence(v runtime.Value, r *Run) shape { literals = append(literals, es.literal) } settled.multi = true + settled.repeated = repeated settled.literal = "(" + strings.Join(literals, ", ") + ")" return settled } @@ -410,6 +419,9 @@ func buildFeatures(req *Request) ([]feature, error) { // First pass: settle each member's shape over every run that supplies it. var names []string shapes := map[string]shape{} + // A repeated element anywhere in a member's runs marks its feature, even + // when a merge settles the shape over values that did not repeat. + repeated := map[string]bool{} for i := range req.Runs { for _, m := range members(req.Runs[i]) { if m.inOf == "" { @@ -421,6 +433,9 @@ func buildFeatures(req *Request) ([]feature, error) { return nil, fmt.Errorf("case %s: parameter %q shares a name with a feature of AnalysisRecords::AnalysisRun", req.Case, m.name) } sh := classify(m.value, &req.Runs[i]) + if sh.repeated { + repeated[m.name] = true + } cur, seen := shapes[m.name] if !seen { names = append(names, m.name) @@ -519,6 +534,7 @@ func buildFeatures(req *Request) ([]feature, error) { } f := feature{name: name} applyShape(&f, shapes[name]) + f.repeated = repeated[name] feats = append(feats, f) if shapes[name].kind == kindQuantity { feats = append(feats, feature{name: name + "Unit", typ: scalarValuesString, unitOf: name}) @@ -527,6 +543,18 @@ func buildFeatures(req *Request) ([]feature, error) { return feats, nil } +// hasRepeatedElement reports whether two elements of a sequence are equal values. +func hasRepeatedElement(elements []runtime.Value) bool { + seen := runtime.NewSet() + for _, element := range elements { + if seen.Contains(element) { + return true + } + seen.Add(element) + } + return false +} + // numericPair reports whether the types are Integer and Real in either order: // one numeric family for the record definition, settling to Real. func numericPair(a, b string) bool { @@ -701,6 +729,9 @@ func checkExisting(req *Request, feats []feature, defName string) error { } return fmt.Errorf("record definition %s declares %s as %s but the run values need %s; record into another package with `into`", def, f.name, kind, want) } + if f.multi && f.repeated && decl.Unique { + return fmt.Errorf("record definition %s declares %s unique but the run values repeat a value; record into another package with `into`", def, f.name) + } if !f.ref && f.typ != "" && decl.TypeFQN != "" && decl.TypeFQN != f.typ && f.typ != scalarValuesScalarValue && decl.TypeFQN != scalarValuesScalarValue { // An Integer literal is valid under a declared Real; the diff --git a/internal/exec/analysis/record/sequence_compat_test.go b/internal/exec/analysis/record/sequence_compat_test.go new file mode 100644 index 0000000000..d7c0f865c6 --- /dev/null +++ b/internal/exec/analysis/record/sequence_compat_test.go @@ -0,0 +1,126 @@ +package record + +import ( + "strings" + "testing" + + "github.com/Open-MBEE/OpenSysML/internal/exec/runtime" + "github.com/Open-MBEE/OpenSysML/internal/syntax/format" +) + +func TestGenerateSequenceMixedQuantityAndNumberFallsBackToText(t *testing.T) { + res, err := Generate(Request{ + Package: "Records", Case: "P::temps", Provenance: provenance(KindRun), + Runs: []Run{{ + Spell: spell(), + Outputs: []runtime.CalcOutputValue{{Name: "temps", Value: seqOf(kelvin(300), integer(301))}}, + }}, + }) + if err != nil { + t.Fatalf("Generate: %v", err) + } + for _, want := range []string{ + "attribute temps : ScalarValues::String;", + `attribute :>> temps = "[300.0 [K], 301]";`, + } { + if !strings.Contains(res.Source, want) { + t.Errorf("source is missing %q:\n%s", want, res.Source) + } + } + if strings.Contains(res.Source, "tempsUnit") { + t.Errorf("mixed quantity/plain sequence unexpectedly has a unit companion:\n%s", res.Source) + } +} + +func TestGenerateExistingSequenceRejectsRepeatedValuesForUniqueMember(t *testing.T) { + _, err := Generate(Request{ + Package: "Records", Case: "P::temps", Provenance: provenance(KindRun), + Runs: []Run{{Spell: spell(), Outputs: []runtime.CalcOutputValue{{Name: "temps", Value: seqOf(realValue(300), realValue(300), realValue(341.2))}}}}, + Existing: Existing{Definition: true, Stem: "temps", Attributes: map[string]Feature{ + "temps": {TypeFQN: scalarValuesReal, Multi: true, Unique: true}, + }}, + }) + if err == nil || !strings.Contains(err.Error(), "declares temps unique but the run values repeat a value") { + t.Fatalf("Generate = %v, want the unique repeated-value refusal", err) + } +} + +// Value equality, not literal spelling, detects a repeat: an Integer 1 and a +// Real 1.0 are one value to a unique member. +func TestGenerateExistingSequenceRejectsARepeatAcrossNumericKinds(t *testing.T) { + _, err := Generate(Request{ + Package: "Records", Case: "P::temps", Provenance: provenance(KindRun), + Runs: []Run{{Spell: spell(), Outputs: []runtime.CalcOutputValue{{Name: "temps", Value: seqOf(integer(1), realValue(1.0))}}}}, + Existing: Existing{Definition: true, Stem: "temps", Attributes: map[string]Feature{ + "temps": {TypeFQN: scalarValuesReal, Multi: true, Unique: true}, + }}, + }) + if err == nil || !strings.Contains(err.Error(), "declares temps unique but the run values repeat a value") { + t.Fatalf("Generate = %v, want the unique repeated-value refusal", err) + } +} + +// A repeat in a later run marks the member even though the merge settles its +// shape over runs that did not repeat. +func TestGenerateExistingSequenceARepeatInALaterRun(t *testing.T) { + cases := map[string]struct { + unique bool + wantErr bool + }{ + "unique member refuses": {unique: true, wantErr: true}, + "nonunique member accepts": {unique: false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + res, err := Generate(Request{ + Package: "Records", Case: "P::temps", Provenance: provenance(KindRun), + Runs: []Run{ + {Spell: spell(), Outputs: []runtime.CalcOutputValue{{Name: "temps", Value: seqOf(realValue(300), realValue(310))}}}, + {Spell: spell(), Outputs: []runtime.CalcOutputValue{{Name: "temps", Value: seqOf(realValue(300), realValue(300))}}}, + }, + Existing: Existing{Definition: true, Stem: "temps", Attributes: map[string]Feature{ + "temps": {TypeFQN: scalarValuesReal, Multi: true, Unique: tc.unique}, + }}, + }) + if tc.wantErr { + if err == nil || !strings.Contains(err.Error(), "declares temps unique but the run values repeat a value") { + t.Fatalf("Generate = %v, want the unique repeated-value refusal", err) + } + return + } + if err != nil { + t.Fatalf("Generate: %v", err) + } + if _, err := format.Source("", []byte(res.Source), format.DefaultOptions); err != nil { + t.Fatalf("generated source does not parse: %v", err) + } + }) + } +} + +func TestGenerateExistingSequenceCompatibility(t *testing.T) { + cases := map[string]struct { + values []runtime.Value + unique bool + }{ + "unique values in unique member": {values: []runtime.Value{realValue(300), realValue(310)}, unique: true}, + "repeated values in nonunique member": {values: []runtime.Value{realValue(300), realValue(300)}, unique: false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + res, err := Generate(Request{ + Package: "Records", Case: "P::temps", Provenance: provenance(KindRun), + Runs: []Run{{Spell: spell(), Outputs: []runtime.CalcOutputValue{{Name: "temps", Value: seqOf(tc.values...)}}}}, + Existing: Existing{Definition: true, Stem: "temps", Attributes: map[string]Feature{ + "temps": {TypeFQN: scalarValuesReal, Multi: true, Unique: tc.unique}, + }}, + }) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if _, err := format.Source("", []byte(res.Source), format.DefaultOptions); err != nil { + t.Fatalf("generated source does not parse: %v", err) + } + }) + } +} diff --git a/internal/frontend/repl/record.go b/internal/frontend/repl/record.go index 6405beeee1..1d266643a8 100644 --- a/internal/frontend/repl/record.go +++ b/internal/frontend/repl/record.go @@ -513,6 +513,7 @@ func recordAttributes(idx *symbols.Index, sem *semantics.Model, def *symbols.Sym f.TypeFQN = idx.GetFQN(types[0]) } f.Multi = !sem.GoverningMultiplicityOf(m).AtMostOne() + f.Unique = sem.IsUnique(m) attrs[m.Name] = f } return attrs diff --git a/internal/frontend/repl/record_sequence_compat_test.go b/internal/frontend/repl/record_sequence_compat_test.go new file mode 100644 index 0000000000..e09cf51686 --- /dev/null +++ b/internal/frontend/repl/record_sequence_compat_test.go @@ -0,0 +1,68 @@ +package repl + +import ( + "testing" + + "github.com/Open-MBEE/OpenSysML/internal/semantic/resolve" + "github.com/Open-MBEE/OpenSysML/internal/semantic/semantics" +) + +func TestRecordAttributesReportsSequenceUniqueness(t *testing.T) { + for name, declaration := range map[string]string{ + "unique": "attribute temps : Real[0..*];", + "nonunique": "attribute temps : Real[0..*] nonunique;", + } { + t.Run(name, func(t *testing.T) { + s := NewSession() + model := `package Records { + private import ScalarValues::*; + private import AnalysisRecords::*; + part def Rec :> AnalysisRecords::AnalysisRun { ` + declaration + ` } + }` + if errs := errorDiagnostics(s.Submit(model).Diagnostics); len(errs) > 0 { + t.Fatalf("model has errors: %v", errs) + } + idx := s.symbolIndex() + defs := idx.LookupQualified("Records::Rec") + if len(defs) != 1 { + t.Fatalf("Records::Rec resolves to %d symbols", len(defs)) + } + resolver := resolve.New(idx) + sem := semantics.NewModel(resolver) + resolver.SetModel(sem) + feature, ok := recordAttributes(idx, sem, defs[0])["temps"] + if !ok || !feature.Multi { + t.Fatalf("temps = %+v (present %v), want a multi-valued feature", feature, ok) + } + wantUnique := name == "unique" + if feature.Unique != wantUnique { + t.Errorf("temps.Unique = %v, want %v", feature.Unique, wantUnique) + } + }) + } +} + +func TestRecordAttributesFollowsInheritedNonunique(t *testing.T) { + s := NewSession() + model := `package Records { + private import ScalarValues::*; + private import AnalysisRecords::*; + part def Base { attribute temps : Real[0..*] nonunique; } + part def Rec :> Base, AnalysisRecords::AnalysisRun { attribute :>> temps; } + }` + if errs := errorDiagnostics(s.Submit(model).Diagnostics); len(errs) > 0 { + t.Fatalf("model has errors: %v", errs) + } + idx := s.symbolIndex() + defs := idx.LookupQualified("Records::Rec") + if len(defs) != 1 { + t.Fatalf("Records::Rec resolves to %d symbols", len(defs)) + } + resolver := resolve.New(idx) + sem := semantics.NewModel(resolver) + resolver.SetModel(sem) + feature, ok := recordAttributes(idx, sem, defs[0])["temps"] + if !ok || !feature.Multi || feature.Unique { + t.Errorf("temps = %+v (present %v), want inherited multi nonunique feature", feature, ok) + } +}