-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Conversation
7fb382d
to
e5fddcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test case to ensure that when multiple defaults arise as a result of unification, this logic still catches it. (It seems unlikely that anyone adds multiple defaults on purpose, so this is actually the case that's most important to check for.)
I can write some example as I understand, it would be nice if we can define a use case together for the real issue, It is probably nice to define use cases when write a user story, TDD would save us some time :). |
|
||
|
||
-- err -- | ||
CType has multiple defaults which is not allowed |
There was a problem hiding this comment.
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
currently
doesn't error but
error out correctly, a tad confusing |
the snippet for the use cases: https://github.com/ying-jeanne/cue_test_cases/blob/bf84db6edba61d3479838967d7445f1775393fc3/main.go a ticket opened on cue-lang because the expr() behaves weirdly cue-lang/cue#1245 |
@@ -0,0 +1,19 @@ | |||
|
|||
This would be the typical cases we should fail, since Bar is refereced to a type with default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wording nit:
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 |
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) |
There was a problem hiding this comment.
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 ops == cue.OrOp { | ||
for _, vval := range vvals { | ||
if inst, path := vval.Reference(); len(path) > 0 { | ||
fmt.Println(">>>>>>>>>>>>>>>>> I am here >>>>>>>>>>>>>>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug output needs removal
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) |
There was a problem hiding this comment.
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
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?? |
There was a problem hiding this comment.
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:
- There's an author of
AOrEnum
, who expressed the intent that the default value of their type bebaz
- There's an author of
Foo
, who is (clearly) directly aware ofAOrEnum
, as they wrote some text explicitly referencing it. Foo
's author, being ostensibly aware ofAOrEnum
, is choosing to supersede the default ofbaz
withohai
.
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:
- Defaults are a native concept in CUE that authors must deal with; no such concept exists in TS
- The primitive kinds in the languages are simply different (disjunction semantics in CUE; existence of enums in TS)
- 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.
There was a problem hiding this comment.
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.
fixes #15