Add "None" to spec #263

rhpvorderman · 2018-11-09T12:36:57Z

After running into yet another case where None is definitely needed (or otherwise causing hideous workarounds) I decided to at it as an example in this pull request.

This aims to implement things that were discussed in #209

orodeh · 2018-11-09T22:28:08Z

I agree, this has been missing for a while. None is, in my opinion, a better choice than Null.

👍

antonkulaga · 2018-11-12T22:07:02Z

I think that you should have started with Some/None support instead of null-s from the very begining

cjllanwarne · 2018-11-13T16:01:56Z

I totally agree with the need for a None literal! Thanks for making this PR.
Instead of only mentioning None all the way down on line 2280 (not your fault - the length of the spec is a known issue...), I think I would add this concept in a new Optional Literals section right after Pair Literals (see line 26 in the ToC) and talk about how any non-optional value can be automatically wrapped into an optional (eg Int? maybe_integer = 5), and how None can be used as a literal to specify no value.
Side comment - for optional File outputs specifically (your example) - I think that eventually we'll want a way of expressing this using an engine function like File? logfile = optional_file("test.log") - this (a) eliminates the need for coordinating behavior through the command and the outputs using the boolean logfile, and (b) works in more general cases where we cannot predict using an if expression which Files will and won't be produced.
- But for now, the example is great,
- And None is useful in all sorts of situations so please don't take this comment as a reason not to add Nones!

versions/development/SPEC.md

rhpvorderman · 2018-11-19T08:09:56Z

@cjllanwarne and @pshapiro4broad thank you for your feedback. I edited the pull request.

What does everyone think about retroactively adding this to the 1.0 spec? I think that we should do this. The concept is already there. But since there is no literal for it you cannot use it easily yet. The change will be backwards compatible.

geoffjentry · 2018-11-19T14:04:38Z

There are no retroactive changes

rhpvorderman · 2018-11-19T14:56:47Z

@geoffjentry . Okay, that's settled then. This pull request only affects the development spec.

geoffjentry · 2018-11-19T15:26:42Z

Yeah we’ve bent the rules a few times for truly inconsequential changes like typos in prose as well as bugs in the grammar file but otherwise trying to not introduce retroactive changes. Thanks

pshapiro4broad · 2018-11-20T15:00:27Z

Thanks for making the changes, it looks better to me. Do you want to include an example that makes it explicit that xxx != None is equivalent to defined(xxx)?

versions/development/SPEC.md

rhpvorderman · 2018-11-21T08:06:10Z

Thanks @yfarjoun @pshapiro4broad and @cjllanwarne for your excellent comments. These should now be addressed.

versions/development/SPEC.md

yfarjoun · 2018-11-21T16:00:06Z

versions/development/SPEC.md

+    test ~{true="--log" false="" log} inputFile outputPath
+  }
+  output {
+    File? logFile = if log then "test.log" else None


is this actually needed? what happens now if you just have

File? logFile = "test.log"

Then logFile will always be test.log even if there is no test.log created.

versions/development/SPEC.md

yfarjoun · 2018-11-26T14:08:02Z

versions/development/SPEC.md

+Boolean test_defined2 = defined(maybe_five_and_is) # Evaluates to true
+Boolean test_is_none = maybe_five_but_is_not == None # Evaluates to true, same as !definedmaybe_five_but_is_not)
+Boolean test_not_none = maybe_five_but_is_not != None # Evaluates to false, same as defined(maybe_five_but_is_not )
+Boolean compare_int_to_none = certainly_five == None # This will cause an error, since None only makes sense when dealing with optionals.


is this a runtime error or a validation error?

I'm not sure that this should be an error.

What about

Int? maybe_five = 5 Int five = 5 Boolean compare_five_to_maybe_five = maybe_five == five

Should this validate?
I think it should and that the answer should be True.

but this means that one should be able to compare an optional with a non-optional.

I have opened #271 for general discussion of typechecking of optional values (i.e. when use of an optional value may be a static validation error, a runtime error in case actually null, or not an error at all). I think the ambiguities involved are broader than this more-focused PR for adding None

As we move to tie off this PR, perhaps we should punt on this example for now?

Agree. Not rushing this in through the backdoor. I will remove this.

versions/development/SPEC.md

geoffjentry · 2018-11-26T18:38:00Z

versions/development/SPEC.md

+The `None` literal is the value that an optional has when it is not defined.
+
+```WDL
+Int? maybe_five_but_is_not = None


Curious what the non-scalawags think - is None intuitive here?

It is to me, but I'm used to Scala's Option, which is what this construct was modeled after.

I don't know that I have any suggestion which seems better (going back to "it is intuitive to me"), perhaps Empty?

In python, there is also a None. It also means something like 'empty' and calling a None variable, will give a not defined error. So for people who know python, it can be intuitive (it is for me, but I also know scala and I can not judge for other people).

I think null is a better choice as that's how you write this concept in JSON, which is how WDL inputs are represented. That way the same concept looks the same in both paces.

I have a deep loathing of null in general, but you raise a good point.

One thing worth thinking through is if there's a meaningful difference between "This space intentionally left blank" and "For unknown reasons there's nothing here" in WDL-land. When those are both viable states I think that null should always mean the latter, and only the latter

I think assigning null to something is a contradiction in itself. So, in my preference null is not available as a variable in the toolbox.

As for the "then it will look the same as in JSON" argument. You have a point there, but frankly, None is different from null. Using the same name for a different concept does not sound like a good idea to me.

One thing worth thinking through is if there's a meaningful difference between "This space intentionally left blank" and "For unknown reasons there's nothing here" in WDL-land. When those are both viable states I think that null should always mean the latter, and only the latter

Can you explain how these two different states could arise in WDL? They sound to me like a states in the mind of the WDL user, not the WDL interpreter.

I think assigning null to something is a contradiction in itself. So, in my preference null is not available as a variable in the toolbox.

Remember that, in my proposal, null is the name WDL has for the concept of an unassigned value. It has no meaning with respect to the implementation of WDL, or pointers, or other programming language concepts.

As for the "then it will look the same as in JSON" argument. You have a point there, but frankly, None is different from null. Using the same name for a different concept does not sound like a good idea to me.

Are you saying that using null in JSON has a different effect than using (as you propose) None in WDL? If so, how is this difference detectable from inside WDL?

In my proposal, null is not different than None as they are just different spellings of the same concept. Maybe the point that you're trying to make is that null has baggage due to its meaning in other programming languages. I agree that it could be a hurdle for WDL users that have experience programming in other languages. Is this the argument that you're making here?

@pshapiro4broad

Can you explain how these two different states could arise in WDL?

I don't have an example, just thought it was worth throwing it out there in case it prompted someone to think of one

In my proposal, null is not different than None as they are just different spellings of the same concept.

I don't think they necessarily are, going back to the difference I cited above. For instance in R NA and NULL are two very difference concepts, but they'd both fold into null in many languages which only have the one concept.

Maybe the point that you're trying to make is that null has baggage due to its meaning in other programming languages.

I certainly fall under that category (I've protested against adding "the billion dollar mistake" to WDL in the past as well), but to go back earlier in the convo I also admit I'm a fairly biased witness here.

@geoffjentry @pshapiro4broad Most of the "Null vs None" stuff was already discussed in #209. Most people liked None more, so I settled for that.

rhpvorderman · 2018-12-06T08:13:23Z

What needs to be done to get this PR ready for voting? I would like to get this into the spec at some point. There's been a lot of discussion about the PR, but I suggest we get down to voting this thing. If it is insufficient, it is back to the drawing board for me. But then at least there is a decision being made.

cjllanwarne · 2018-12-12T16:28:30Z

@rhpvorderman my only complaint with this PR as-is is the use of an optional file output in one of the examples (ie File? maybe_file = ).

I think I'd personally rather keep this PR as the title suggests (allowing None to be used as a literal).

I definitely think that optional File outputs should work - but I'd rather make that explicit in a separate PR which can go through refinement in case there's a nicer way to express it. I'm mostly concerned that we don't let File? delocalization implementation issues on cloud platforms slow down the implementation and adoption of a should-be-unrelated literal for empty optionals.

cjllanwarne · 2018-12-12T16:34:04Z

versions/development/SPEC.md

+    test ~{true="--log" false="" log} inputFile outputPath
+  }
+  output {
+    File? logFile = if log then "test.log" else None


This is the line which I think could be trickier to guarantee. I think for this PR that this example should be removed or reworked to not use File?, and a new PR specifically guaranteeing support for File? outputs should be added (in case there are alternative ways or syntax options of expressing this intention in an easier-for-cloud-support way)

yfarjoun · 2018-12-12T16:51:07Z

versions/development/SPEC.md

+## Document
+
+```
+$document = ($import | $task | $workflow)+


Can you really have more than one workflow in a single document?

also, why are we making these additions in this PR? seems out of place.

This looks like an accidental reversal of #270 happened during a rebase to me

(or maybe this PR just need another rebase on master to pick up that missing PR?)

I definitely did not put that in 🙂. I will try if a rebase fixes it.

@rhpvorderman Heads up that I don't think your rebasing did the trick as this line is still here.

cjllanwarne · 2018-12-18T01:32:08Z

versions/development/SPEC.md

@@ -23,6 +23,8 @@
    * [Map Literals](#map-literals)
    * [Object Literals](#object-literals)
    * [Pair Literals](#pair-literals)
+    * [Optional Literals](#optional-literals)
+  * [Document](#document)


Still needs to be deleted

cjllanwarne · 2018-12-18T01:32:40Z

versions/development/SPEC.md

+If `log` is true, the command will evaluate to `test --log inputFile outputPath` and `logFile` will be `test.log`.
+If `log` is false, the command will evaluate to `test inputFile outputPath` and `logFile` will be `None`.
+
+## Document


Still needs to be deleted

rhpvorderman · 2018-12-18T08:22:51Z

I personally don't think that saying "git rebases are hard" is a good reason to push ahead despite these concerns

[tangent] Squashing and rebasing are gruesome practices that destroy git history. It is nice in some circumstances, but when overused you get problems like I have had here. Having to clean up all sorts of stuff that I never wrote from my PR. This was a lot of tedious and unnecessary work. Maybe this was because my rebasing went wrong because I am not a git guru. But in general why resort to guru stuff when safer and easier ways are available? But let's have that discussion elsewhere. I just wanted to note that "git rebases are hard" is an oversimplification of my viewpoint in this matter. [/tangent]

With the exception of the File? output example, I'm very much in favor of the addition of a None literal. However I would not vote for this PR as it is right now

Thanks @cjllanwarne for providing a list of arguments why rushing in optional files through the backdoor of this PR is a bad idea. You convinced me. I have removed the optional file example.

cjllanwarne · 2018-12-18T15:38:12Z

Thanks @rhpvorderman! And FWIW I agree with your meta comment that if something is making iteration on these PRs difficult for contributors, that's definitely worth thinking about to see if there's anything in the git flow to make that easier.

@geoffjentry my concerns are met, happy to vote on this PR now.

rhpvorderman · 2018-12-19T14:25:39Z

@mlin I have removed the typechecking example from the PR.

geoffjentry · 2019-01-07T16:17:03Z

Sorry for the delay on this, I've been out of office for the last few weeks. Opening for a vote now.

orodeh · 2019-01-07T16:31:32Z

👍

mwalker174 · 2019-01-07T16:44:25Z

versions/development/SPEC.md

+
+Boolean test_defined = defined(maybe_five_but_is_not) # Evaluates to false
+Boolean test_defined2 = defined(maybe_five_and_is) # Evaluates to true
+Boolean test_is_none = maybe_five_but_is_not == None # Evaluates to true, same as !definedmaybe_five_but_is_not)


Missing a left parenthesis after defined.

Hi @mwalker174 - unfortunately once voting is active votes are on the PR as-is. I'd encourage either you or @rhpvorderman file a followup PR after this one is finished

mwalker174 · 2019-01-07T16:54:29Z

👍

aednichols · 2019-01-07T23:09:24Z

👍

DavyCats · 2019-01-08T08:11:36Z

👍

mlin · 2019-01-08T18:11:53Z

👍

cjllanwarne · 2019-01-08T19:22:40Z

👍

cjllanwarne · 2019-03-06T20:37:06Z

@geoffjentry - This PR has been implemented. No implementation issues came up. This PR can now be merged into the SPEC.

pshapiro4broad reviewed Nov 16, 2018

View reviewed changes