Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Blocked] Check unique default before using the value #26

Closed
wants to merge 3 commits into from
Closed

[Blocked] Check unique default before using the value #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84e095b
check default before return
ying-jeanne Aug 31, 2021
918b679
rename txtar files and adding some use cases
ying-jeanne Sep 1, 2021
0754b5e
correct enum cases
ying-jeanne Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions encoder/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,19 @@ func loadCases(dir string) ([]Case, error) {
if len(a.Files) != 2 {
return nil, fmt.Errorf("Malformed test case '%s': Must contain exactly two files (CUE and TS/ERR), but has %d", file, len(a.Files))
}
if strings.HasSuffix(fi.Name(), "error") {

fname := strings.TrimSuffix(fi.Name(), filepath.Ext(fi.Name()))
if strings.HasSuffix(fname, "error") {
cases = append(cases, Case{
CaseType: ErrorType,
Name: fi.Name(),
Name: fname,
CUE: string(a.Files[0].Data),
ERROR: strings.TrimSuffix(string(a.Files[1].Data), "\n"),
})
} else {
cases = append(cases, Case{
CaseType: TSType,
Name: fi.Name(),
Name: fname,
CUE: string(a.Files[0].Data),
TS: string(a.Files[1].Data),
})
Expand Down
59 changes: 54 additions & 5 deletions encoder/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,13 +181,16 @@ func (g *generator) genType(name string, v cue.Value) {

tvars["tokens"] = tokens

d, ok := v.Default()
d, ok, err := validateAndGetDefault(v)
if err != nil {
g.addErr(err)
}

if ok {
dStr, err := tsprintField(d)
g.addErr(err)
tvars["default"] = dStr
}

// TODO comments
// TODO maturity marker (@alpha, etc.)
g.exec(typeCode, tvars)
Expand Down Expand Up @@ -220,7 +223,7 @@ func (g *generator) genEnum(name string, v cue.Value) {
g.addErr(err)
}
pairs = orPairs
defaultValue, err = getDefaultValue(v)
defaultValue, err = getDefaultEnumValue(v)
if err != nil {
g.addErr(err)
}
Expand All @@ -243,8 +246,50 @@ func (g *generator) genEnum(name string, v cue.Value) {
g.exec(enumCode, tvars)
}

func getDefaultValue(v cue.Value) (string, error) {
func validateAndGetDefault(v cue.Value) (cue.Value, bool, error) {
defs := []cue.Value{}
ops, vvals := v.Expr()
fmt.Printf("........ the v expr() is: %v and ops is : %v\n", vvals, ops)
if ops == cue.OrOp {
for _, vval := range vvals {
if inst, path := vval.Reference(); len(path) > 0 {
fmt.Println(">>>>>>>>>>>>>>>>> I am here >>>>>>>>>>>>>>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug output needs removal

if def, ok := inst.Lookup(path...).Default(); ok {

defs = append(defs, def)
}
}
}
}

def, ok := v.Default()
if ok {
defs = append(defs, def)
label, _ := v.Label()
op, dvals := def.Expr()
if len(dvals) > 1 && op == cue.OrOp {
return def, true, valError(v, "%s has multiple defaults which is not allowed", label)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message should print out the defaults, to improve failure locality (the cost of mapping a failure to a particular place in code). Ideally, it'd even emit line numbers, like native CUE errors do, but that's more than we need to worry about right now.

}
}
if len(defs) >= 1 {
for _, def := range defs[1:] {
if !defs[0].Equals(def) {
label, _ := v.Label()
return def, true, valError(v, "%s has multiple defaults which is not allowed", label)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as other comment, print out the conflicting default values in the error message

}
}
return defs[0], true, nil
}
return def, false, nil

}

func getDefaultEnumValue(v cue.Value) (string, error) {
def, ok, err := validateAndGetDefault(v)
if err != nil {
return "", err
}

if ok {
if v.IncompleteKind() == cue.StringKind {
dStr, err := tsprintField(def)
Expand Down Expand Up @@ -475,7 +520,11 @@ func (g *generator) genInterface(name string, v cue.Value) {

kv := KV{K: k, V: vstr}

d, ok := fields.Value().Default()
// When default exist but have sevarals, we need to fail
d, ok, err := validateAndGetDefault(fields.Value())
if err != nil {
g.addErr(err)
}
// [...number] results in [], which is not desired
// TODO: There must be a better way to handle this
if ok && d.IncompleteKind() != cue.ListKind {
Expand Down
19 changes: 19 additions & 0 deletions encoder/tests/conflictEnumDeferror.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@

This would be the typical cases we should fail, since Bar is refereced to a type with default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wording nit:

Suggested change
This would be the typical cases we should fail, since Bar is refereced to a type with default
This would be the typical cases we should fail, since Bar references a type with a default

and we are trying to assign another default

Seems CUE is picking the default before eval it, it seems we can get the reference first and
check whether default exist there, if yes if the default also exist for the current cue object error out

Or should we just forbidden the disjunction between Enum and other types??
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really interesting question! It should probably be a comment in the PR, or even its own issue, rather than hiding it in an actual committed file, i think.

My intuition is that, first and foremost, we have to follow the constraints imposed by TypeScript's own rules. From there, we should do the least magical thing possible. Given that, my initial thought is to allow this, as:

export enum AOrEnum: {
  Foo: 'foo',
  Bar: 'bar',
  Baz: 'baz'
}

export interface Foo {
  Bar: AOrEnum | "ohai"
}

export const defaultFoo: Foo = {
  Bar: "ohai"
}

At first, this seemed reasonable, because of what i felt i could infer about authorial intent:

  1. There's an author of AOrEnum, who expressed the intent that the default value of their type be baz
  2. There's an author of Foo, who is (clearly) directly aware of AOrEnum, as they wrote some text explicitly referencing it.
  3. Foo's author, being ostensibly aware of AOrEnum, is choosing to supersede the default of baz with ohai.

However, going through this exercise led me to realize that this case is narrow and circumstantial, not based on generalizable principles of the sort people can reason about. That's because we're bumping into fundamental differences in the models on which CUE and TS are based:

  1. Defaults are a native concept in CUE that authors must deal with; no such concept exists in TS
  2. The primitive kinds in the languages are simply different (disjunction semantics in CUE; existence of enums in TS)
  3. While both TS and CUE are mostly structurally typed, which generally it possible to flow types through the system more freely than if names must strictly align, we're (necessarily) making that harder by trying to snapshot bits of the system to translate from CUE to TS, without ever being able to guarantee that we have the whole type graph to examine for completeness or correctness

i don't know what our good answers are here, yet, but the above sort of reasoning makes me reasonably sure there simply aren't any, and we shouldn't waste time or effort searching for them. Instead, we should be very conservative in what we support, erroring aggressively when the slightest bit of ambiguity arises about how to e.g. handle a default or flow an enum into an interface, and then consider opening up further when use cases present themselves.

That's still not a clear, categorical rule we can apply easily, but...well, it's a general basis for saying no and constraining scope, which is usually a safe place to be.

Copy link
Contributor

@sdboyer sdboyer Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All that said, it's worth noting that CUE seems to follow the same logic about user intent/awareness that i originally ran with - see this playground, and note that C has a single default, but D does not. Which is...actually really interesting, and potentially useful.

Now Enum | Enum is not export at all, should we support Enum | Enum?
-- cue --
package cuetsy

AOrEnum: "foo" | "bar" | *"baz" @cuetsy(kind="enum")
Foo: {
Bar: AOrEnum | *"ohai"
} @cuetsy(kind="interface")

-- err --
Bar has multiple defaults which is not allowed
15 changes: 15 additions & 0 deletions encoder/tests/conflictInterfaceDeferror.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
-- cue --
AType: *4 | int @cuetsy(kind="type")
Foo: {
Baz: AType | *7
} @cuetsy(kind="interface")

-- err --
export type AType = number;
export const aTypeDefault: AType = 4
export interface Foo {
Baz: number;
}
export const fooDefault: Foo = {
Baz: 7,
}
8 changes: 8 additions & 0 deletions encoder/tests/conflictTypeDeferror.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-- cue --
AType: "foo" | *"bar" | "baz" @cuetsy(kind="type")
BType: "foo" | "bar" | *"baz" @cuetsy(kind="type")
CType: AType | BType @cuetsy(kind="type")


-- err --
CType has multiple defaults which is not allowed
Copy link
Collaborator Author

@ying-jeanne ying-jeanne Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is making sense and erring out now

0 encoder/tests/defaultsEnum → encoder/tests/defaultsEnum.txtar
View file
Open in desktop
File renamed without changes.
0 encoder/tests/defaultsInterface → encoder/tests/defaultsInterface.txtar
View file
Open in desktop
File renamed without changes.
0 encoder/tests/interfaces → encoder/tests/interfaces.txtar
View file
Open in desktop
File renamed without changes.
0 encoder/tests/mismatchLenerror → encoder/tests/mismatchLenerror.txtar
View file
Open in desktop
File renamed without changes.
0 encoder/tests/namelessNumericserror → encoder/tests/namelessNumericserror.txtar
View file
Open in desktop
File renamed without changes.
0 encoder/tests/orEnum → encoder/tests/orEnum.txtar
View file
Open in desktop
File renamed without changes.
0 encoder/tests/structEnumerror → encoder/tests/structEnumerror.txtar
View file
Open in desktop
File renamed without changes.