brownplt / pyret-lang

The Pyret language.
Other
1.07k stars 111 forks source link

`spy` and expressions #1177

Open shriram opened 7 years ago

shriram commented 7 years ago

It's a bit annoying that spy takes only names (as opposed to anonymous expressions). That makes it a not-so-good substitute for print, which allowed arbitrary expressions. To view an expression's value, I now have to create a new identifier and bind it, which is completely unnecessary for something very temporary.

blerner commented 7 years ago

No you don't. You can write spy <optional title>: "some descriptive name" : any-expression end.

Joe has documented this; I don't see it merged yet, though.

shriram commented 7 years ago

Eh? I'm talking about being able to write

spy:
  some-id, (expr * involving) - some-other-id
end
blerner commented 7 years ago

So that's utterly useless and undescriptive -- how can you tell the difference between the two things being output? Do you happen to know the order they print in? I'm saying you need to write

spy:
  some-id,
  "the complex expression is": (expr * involving) - some-other-id
end

You don't have to let bind it, but you do have to give it a name so that the spy block can render your output in a readable way.

Identifiers are a special case, because they already have a name, so instead of saying something stupid like "some-id" : some-id, which is redundant, you can just write some-id.

shriram commented 7 years ago

Okay, I didn't guess that, and not having any docs, had no way of knowing what to write instead.

Incidentally, this form is not suggested by the docs you just sent me. That suggests introducing an identifier rather than just a string (which is cognitively less work since the syntax is more permissive and I don't have to worry about shadowing, etc.).

blerner commented 7 years ago

Sorry, I miswrote. You currently need to give a valid identifier name, not a string there; it's grammatically required for now, and I don't recall whether it causes any troubles to loosen it to a string. Regardless, the name there is not checked for shadowing.

shriram commented 7 years ago

It's weird that it wouldn't be checked for shadowing, and more to the point, there's no way a user would guess that — so they're going to end up putting in the effort to think up a new name regardless… I rather prefer the string version.

blerner commented 7 years ago

Yes, it would be nice. And I think it caused problems when we tried to figure out what it would mean. In particular, we were trying to make the common case, spy: some-variable-name end as terse as possible. Which can easily imply that any expression (maybe required to be a simple expression, maybe not) could go there, at which point we have no descriptive names. And if we allowed spy: "foobar" end, then it's really confusing to me what the semantics should be -- is that a parse error because you have a label without a spied-upon value, a simple-case spy with a name that's the same as the (constant) value being spied upon, a parse error because you have a value without a name, or something else?

shriram commented 7 years ago

I guess I'm proposing two forms for each spy "term":

separated by commas (with an optional extra comma at the end).

Is that also problematic? I figured the reason you weren't allowing "naked" expressions was it would be too ambiguous, and I can see that. Does the above work better? I'm basically going back to the form you mentioned earlier in this bug report thread.

blerner commented 7 years ago

(edited your comment because the formatting wiped out what you were saying. thanks markdown...)

I don't recall exactly why we decided on identifiers-only, but I do know we discussed it. It's not in the slack channel history, so it must be in email somewhere...

I don't think I aesthetically like strings there, though, tbh: in the common case, you don't need multiple words, so hyphenation is moot, so the quotes around the strings are just line noise. And, since spies are visually similar to tables, having strings there makes the spy syntax needlessly different from table syntax (I'm thinking of extends syntax, in particular) where you have column names on the left side of the colons. I understand that in general, having full strings there is more flexible. I just don't recall the full arguments against it.

jpolitz commented 7 years ago

There were two main arguments for identifiers and labelled expressions from what I remember:

After writing the docs, I really wanted to be able to do

spy: expr end

It seems so obviously like something folks will try that we should support it.

We could make spy-field be

spy-field:
  | <expr>
  | <label> : <expr>
end

And then it's just a matter of picking the name on the left for the “anonymous” entries. It could even be the pretty-printing of the AST, truncated at, say, 20 characters (which would be the same as the current behavior for identifiers). I could see it being powerful to witness the result of the expression in a spy table for something like lst.length where the table would actually show:


| lst.length()  |    2             |
| lst           | [list: "a", "b"] |

I think it's easy to generalize the thing to the left of the colon to a string if we want it. I'd go so far as to say we should allow <id> | <string> there if possible.

I don't think the visual similarity between spies and tables should drive the design of spies – I think it's a potential source of confusion and wish they looked less similar. I'm mildly dissatisfied with the current rendering in CPO because it uses space badly, as well. It's quite a bit better than not having spies, though :-) I just don't think the visual similarity should be a reason to make a design decision for spies.

shriram commented 7 years ago

The bit about tables confused me and I don't really see it as a design criterion. Overall I pretty much agree w/ with @jpolitz wrote.

blerner commented 7 years ago

So then what would spy: "foobar" end produce? Is that an anonymous expression labeled by itself; an anonymous expression labeled by its truncated AST, an incomplete label : expr expression? I really don't want to allow SPY COLON binop-expr END as a legal grammatical form. "Scaling spy blocks down" to a single expression just kinda breaks down.

I want to push on this, partly due to my experience at MS working on Windows event tracing: if a value is worth debug-logging, it's worth labeling. I'd rather train our students to have to write spy: <some description>: some(expression) end than just spy: some(expression) end and think through what they're spying on, than encourage free-form spying.

jpolitz commented 7 years ago

The answer to the first part is straightforward, it would produce "foobar": "foobar", or the table-based drawing of that in the two columns. They'd look identical at the CLI, and maybe be different colors (one syntax-colored, one value-colored) in the browser.

MS working on Windows event tracing: if a value is worth debug-logging, it's worth labeling

I think this is a good software engineering point.

However, the more I think about this, the more I think not being able to do spy: expr end is going to be hugely frustrating and surprising to our users. I don't want to be explaining the virtues of mandatory labels the first time I show them how to display an intermediate value. In addition, it's at least labelled by the source location, which is a huge benefit over just a function call already.

Do you disagree with the use of the expression/value table? I find that quite compelling.

jpolitz commented 7 years ago

(Note that allowing expressions is strictly enabling more syntax, so we can release this version and later expand its capabilities; this doesn't necessarily need to be a blocking discussion.)

blerner commented 7 years ago

Yes, I do disagree. If a student can't enunciate what the expression is doing, they don't understand it. So why are they logging it?

And I very much dislike "foobar" : "foobar" as the result of that rendering. That's practically dadaist. And it flies in the face of the only use I'd have for such a thing: the typical printf "Here 1", printf "Here 2" sequence of mediocre tracing/debugging triggers. I don't want to see it twice.

AND it's utterly bizarre when you recall that spy blocks can be labeled:

spy "foobar": end

is an empty-but-labeled spy block, but

spy: "foobar" end

is an anonymous, stuttering spy block, and

spy: "foobar" : "baz" end

is an anonymous spy block with one labeled expression, and

spy "foobar": "baz" end

is a named, stuttering spy block.

That's way too subtle the (mis-)use of a colon for my taste.

jpolitz commented 7 years ago

Yes, I do disagree. If a student can't enunciate what the expression is doing, they don't understand it. So why are they logging it?

Err... to understand what value it produces, so they can enunciate what it's doing?

And I hardly think naming lst.length() with e.g. l is a revelatory act that demonstrates more understanding than they had before.

it flies in the face of the only use I'd have for such a thing [...] I don't want to see it twice.

You can (only) get the behavior you want in either design by putting the message before the colon and having an empty spy body. I don't see why this is an argument against.

utterly bizarre

That seems like an overstatement.

Do you disagree that users will be frustrated by not being able to write an expression? Is this a context (debugging a known-broken program, trying to get more information) where it's a useful moment to introduce frustration?

blerner commented 7 years ago

I disagree. I can write a gobbledygook expression and have it produce 16. I'm not going to spy on it to figure out that it produces 16; I want to figure out why it isn't producing some meaningful value for me. I don't think naming lst.length() something deliberately obtuse like l is going to help, sure. But I do think that if we're introducing a debugging construct to the language, then yes we should force students to pause and think before reacting. The whole point of the DR is to introduce methodical, deliberate attention to how the functions are built. I'd like to see a student come to me with some spy statement named l so I can tell them, "I don't know what that means; what is its purpose? Can you describe it better?" and watch them struggle to figure out why they're struggling at that particular point in the code.

Orthogonally, yes, I agree that the only way I'd get the singleton string I want is to label the spy block and not an expression itself. But the detailed differences between the four options I showed are way too subtle for the placement of a single colon.

jpolitz commented 7 years ago

I'm happy to leave it as is for now, I think we understand where we disagree.

Let me offer a prediction: We're going to be doing a lot of explaining to frustrated students and teachers who want it to just work. I'm curious to see if (a) that's right or if I'm way off, and (b) if there's any way at all to get a handle on if it was valuable to make them struggle.

I'd also offer that a difference in our perspectives is that I don't view naming lst.length() with something like l as deliberately obtuse. It's just the result of it being the least typing required to get the result of lst.length() to print. I'll probably be doing the equivalent of that a lot when writing programs with spies.

shriram commented 7 years ago

While I like the MS point, I think this slightly conflates two different things: viewing something temporarily and logging.

I think it's a great point when applied to logging, since a log is meant to be something you can view at a temporal remove. In fact, you may view it after the very system you're viewing it for has ceased to exist, in which case you literally can't reconstruct what that value is. Also, logs usually go silently into a hidden channel.

Whereas spy is immediately visible, and annoying enough (as is any debug print output: not a statement about spy specifically) that it's not going to last for long in a program, so it doesn't have to deal with long-term readability.