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

ying-jeanne · 2021-08-31T09:50:45Z

fixes #15

sdboyer · 2021-08-31T15:21:47Z

nit: please use keywords - e.g. "fixes #15" - so that the merging of the PR will auto-close the issue. Or, if it doesn't completely address the issue and therefore shouldn't close it, just a "relates to #15" is good. Having the bare issue number is a bit ambiguous.

sdboyer

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.)

encoder/tests/conflictEnumDeferror

encoder/tests/conflictInterfaceDeferror

encoder/tests/conflictTypeDeferror

ying-jeanne · 2021-09-01T08:29:19Z

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 :).

ying-jeanne · 2021-09-01T11:01:09Z

encoder/tests/conflictTypeDeferror.txtar

+
+
+-- err --
+CType has multiple defaults which is not allowed


this is making sense and erring out now

ying-jeanne · 2021-09-01T12:42:49Z

currently

AType: *4 | int @cuetsy(kind="type")
Foo: {
  Baz: AType | *7
} @cuetsy(kind="interface")

doesn't error but

AType: *4 | int @cuetsy(kind="type")
BType: *7 | int @cuetsy(kind="type")
Foo: {
  Baz: AType | BType
} @cuetsy(kind="interface")

error out correctly, a tad confusing

ying-jeanne · 2021-09-02T09:09:34Z

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

sdboyer · 2021-09-07T01:29:48Z

encoder/tests/conflictEnumDeferror.txtar

@@ -0,0 +1,19 @@
+
+This would be the typical cases we should fail, since Bar is refereced to a type with default


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

sdboyer · 2021-09-07T01:33:35Z

encoder/generator.go

+		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)


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.

sdboyer · 2021-09-07T01:33:54Z

encoder/generator.go

+	if ops == cue.OrOp {
+		for _, vval := range vvals {
+			if inst, path := vval.Reference(); len(path) > 0 {
+				fmt.Println(">>>>>>>>>>>>>>>>> I am here >>>>>>>>>>>>>>")


debug output needs removal

sdboyer · 2021-09-07T01:34:21Z

encoder/generator.go

+		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)


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

sdboyer · 2021-09-07T03:04:50Z

encoder/tests/conflictEnumDeferror.txtar

+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??


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 be baz

There's an author of Foo, who is (clearly) directly aware of AOrEnum, as they wrote some text explicitly referencing it.

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:

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.

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.

ying-jeanne force-pushed the 15 branch 2 times, most recently from 7fb382d to e5fddcb Compare August 31, 2021 09:57

check default before return

84e095b

ying-jeanne force-pushed the 15 branch from e5fddcb to 84e095b Compare August 31, 2021 10:00

ying-jeanne changed the title ~~check default before return~~ Check unique default before using the value Aug 31, 2021

ying-jeanne marked this pull request as ready for review August 31, 2021 10:01

ying-jeanne requested review from sdboyer and sh0rez August 31, 2021 10:01

ying-jeanne linked an issue Aug 31, 2021 that may be closed by this pull request

Error out when attempting to generate a default for a disjunction with multiple defaults #15

Open

sdboyer suggested changes Aug 31, 2021

View reviewed changes

encoder/tests/conflictEnumDeferror Outdated Show resolved Hide resolved

encoder/tests/conflictInterfaceDeferror Outdated Show resolved Hide resolved

encoder/tests/conflictTypeDeferror Outdated Show resolved Hide resolved

rename txtar files and adding some use cases

918b679

ying-jeanne force-pushed the 15 branch from 1ceee71 to 918b679 Compare September 1, 2021 10:42

ying-jeanne commented Sep 1, 2021

View reviewed changes

correct enum cases

0754b5e

ying-jeanne force-pushed the 15 branch from 7b0ff9c to 0754b5e Compare September 2, 2021 08:30

ying-jeanne changed the title ~~Check unique default before using the value~~ [WIP] Check unique default before using the value Sep 2, 2021

ying-jeanne changed the title ~~[WIP] Check unique default before using the value~~ [Blocked] Check unique default before using the value Sep 2, 2021

sdboyer suggested changes Sep 7, 2021

View reviewed changes

ying-jeanne marked this pull request as draft September 16, 2021 14:04

sdboyer closed this May 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ying-jeanne commented Aug 31, 2021 •

edited

sdboyer commented Aug 31, 2021

sdboyer left a comment

ying-jeanne commented Sep 1, 2021

ying-jeanne Sep 1, 2021 •

edited

ying-jeanne commented Sep 1, 2021

ying-jeanne commented Sep 2, 2021

sdboyer Sep 7, 2021

sdboyer Sep 7, 2021

sdboyer Sep 7, 2021

sdboyer Sep 7, 2021

sdboyer Sep 7, 2021

sdboyer Sep 7, 2021 •

edited

		@@ -0,0 +1,19 @@

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

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

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

Conversation

ying-jeanne commented Aug 31, 2021 • edited

sdboyer commented Aug 31, 2021

sdboyer left a comment

Choose a reason for hiding this comment

ying-jeanne commented Sep 1, 2021

ying-jeanne Sep 1, 2021 • edited

Choose a reason for hiding this comment

ying-jeanne commented Sep 1, 2021

ying-jeanne commented Sep 2, 2021

sdboyer Sep 7, 2021

Choose a reason for hiding this comment

sdboyer Sep 7, 2021

Choose a reason for hiding this comment

sdboyer Sep 7, 2021

Choose a reason for hiding this comment

sdboyer Sep 7, 2021

Choose a reason for hiding this comment

sdboyer Sep 7, 2021

Choose a reason for hiding this comment

sdboyer Sep 7, 2021 • edited

Choose a reason for hiding this comment

ying-jeanne commented Aug 31, 2021 •

edited

ying-jeanne Sep 1, 2021 •

edited

sdboyer Sep 7, 2021 •

edited