Position/locate/pseudocode revamp

[pzinn]

• The positioning of code has been greatly improved, allowing for correct location/quoting code/error. compare say

x -> "blah
blah"

before and after. Various hacks that are no longer necessary have been correspondingly removed.

• The Pseudocode type has been revamped. It has been split into two (Pseudocode and PseudocodeClosure), has been renamed in d for consistency and also to avoid confusion (the old name CodeClosure didn't make it clear that the type only existed for debugging purposes, not for execution). disassemble has been retired: all pseudocode is automatically disassembled and displayed in a more human-friendly way.
• tempPosition has been introduced when exporting symbols in a package, so their position is only determined when the symbol is actually used.
• some code cleanup

This PR is the first part of a series involving debugging and error messages. Hopefully it is self-contained.

[pzinn]

weird, builds fine on my comp.

[pzinn]

another issue, inevitably I'll have to fix the output of ComputationsBook tests to reflect the improved code location. I know I've asked before (@mahrud), but how do I populate the ComputationsBook tests when using cmake?

[pzinn]

another issue, inevitably I'll have to fix the output of ComputationsBook tests to reflect the improved code location. I know I've asked before (@mahrud), but how do I populate the ComputationsBook tests when using cmake?

aha, I think I found an old thread:

not directly related to this PR: @mahrud I'm struggling to fix a test, but I'm running blind because locally I can't get the ComputationsBook tests to run; when I do ctest, I get error messages about the .trim files not existing. I vaguely remember having had this problem, but don't remember how to fix it.

You have to run ninja M2-tests or something like that first. I'm not proud of it but couldn't figure out how to make it work with ctest alone ...

maybe this should be in the wiki

[mahrud]

It's two steps:

ninja M2-tests-ComputationsBook -- generate output files
ctest -R  ComputationsBook -- compare against expected output files

[mahrud]

@d-torrance and I have talked a bit about commit message and granularity standards for Macaulay2. I think we both prefer more granular commits (the first commit could have been several bite sized ones), for instance because it's easier to figure out what change broke something using git bisect.

I don't necessarily want you to go back and split the commit, but on the other hand I'd like to push for better commit message standards, especially outside of personal packages:

Position/locate/pseudocode revamp
change compile order
and again
moved pseudocodeClosureClass def down, fixed test
fix test

Could you squash all your commits into one? I think that would be the easiest thing to do for now.

[pzinn]

It's two steps:

ninja M2-tests-ComputationsBook -- generate output files
ctest -R  ComputationsBook -- compare against expected output files

maybe include this in https://github.com/Macaulay2/M2/wiki/Building-M2-from-source-using-CMake

[pzinn]

OK I'll move things around and then force push into a single commit

[mahrud]

@d-torrance I already reviewed the changes here, but it involves the interpreter so probably two reviews would be more appropriate. Feel free to merge if you approve.

[d-torrance]

Yes, I definitely plan on reviewing this (and #3382) soon.

[d-torrance]

I'm hesitant about the changes to how pseudocode is displayed. While the current lisp-like display can be hard to read, the parentheses are great for delineating each Code element.

For example, consider the following current behavior:

i1 : disassemble pseudocode(() -> if true then for x in {} do while true do try null)

o1 = (function restargs: false numparms: 0 framesize: 0 frameID: 8611 (if (global-fetch 218) then: (for in: (list ) from: (null) to: (null) when: (null) list:
     (null) do: (while (global-fetch 218) do: (try (global-fetch 220) (null) (null)))) else: (null)))

Versus the proposed behavior:

i1 : pseudocode(() -> if true then for x in {} do while true do try null)

o1 = function {restargs false                                           }
              {numparms 0                                               }
              {framesize 0                                              }
              {frameID 8894                                             }
              {body if {global-fetch 219                               }}
              {        {then for {framesize 1                         }}}
              {        {         {frameID 8895                        }}}
              {        {         {in list {}                          }}}
              {        {         {from                                }}}
              {        {         {to                                  }}}
              {        {         {when                                }}}
              {        {         {list                                }}}
              {        {         {do while {global-fetch 219         }}}}
              {        {         {         {do try {global-fetch 221}}}}}
              {        {         {         {       {then            }}}}}
              {        {         {         {       {else            }}}}}
              {        {else                                           }}

o1 : PseudocodeClosure

With the proposed behavior, it's not clear that if, for, while, and try are actually the starts of new Code objects, but there are opening parentheses in the current behavior that tell us that.

Also, in an editor like Emacs, we can move our cursor to a closing parenthesis and it will tell us where the corresponding opening parenthesis is with the current behavior. But in the proposed behavior, there are a bunch of curly braces everywhere.

Maybe a compromise would be to keep the basic lisp style, but with proper indentation? Maybe something like the following, which is just the current behavior as indented by Emacs in elisp mode:

(function restargs: false
	  numparms: 0
	  framesize: 0
	  frameID: 8611
	  (if (global-fetch 218)
	      then:
	    (for in: (list )
		 from: (null)
		 to: (null)
		 when: (null)
		 list: (null)
		 do: (while (global-fetch 218)
		       do: (try (global-fetch 220)
				(null)
				(null))))
	    else: (null)))

Also, maybe we could define disassemble Pseudocode := net or similar for backward compatibility?

[mahrud]

Oh I didn't realize this is the display in Emacs. The html display is very different: #3354 (comment)

For some reason I thought the new output was of type Descent, or a similar traversable data structure, but if it's not then I agree with Doug that all the braces are counterproductive.

I'm wondering maybe the following would be the most useful:

• pseudocode should return a PseudocodeClosure object which can be turned back into a FunctionClosure using value
• disassemble should return a nested lisp-style sequence or even something like json.

The second item can happen later, but it's very useful for modern editors.

[pzinn]

Yeah the display uses VerticalList which is ugly in standard mode. (the reason it looks very different -- and nicer -- in webapp mode is a separate story -- I did try to imitate the standard mode but failed).
I can easily make it look like you want @d-torrance but do you really want all the parentheses?? and turn pseudocode into lisp...
as to all your suggestions to reinstate disassemble, that kinda goes again the philosophy of this PR. Also I doubt we need backward compatibility, are there users who ever used this stuff?

[pzinn]

also do you want the "null"s displayed? this is something that M2 is not very consistent about across situations and modes...

[pzinn]

also, I object to all the unncessary colons.

[pzinn]

actually, correction, there does seem to be something wrong with the parenthesising. investigating.

[mahrud]

ugh... this is ugly as hell but sure one can use S-expressions...

I mean, this is how almost any interpreter works! That's the whole point, and in some languages you can explicitly view this:

julia> Meta.show_sexpr(Meta.parse("4 + 4 / 2"))
(:call, :+, 4, (:call, :/, 4, 2))

[pzinn]

None of these changes have anything to do with how the interpreter works. In any case, I've changed the parentheses which should match more closely the required form.

[mahrud]

I think the parentheses/braces between toString and disassemble are off still. Can you explain why you prefer disassemble to produce a sequence possibly containing lists which possibly contain another Pseudocode? What's the difference between CodeList and CodeSequence?

i1 : debug Core; load "Macaulay2Doc/demo1.m2"; g 2
../../Macaulay2/packages/Macaulay2Doc/demo1.m2:8:11:(3):[2]: error: division by zero
../../Macaulay2/packages/Macaulay2Doc/demo1.m2:8:11:(3):[2]: --entering debugger (type help to see debugger commands)
../../Macaulay2/packages/Macaulay2Doc/demo1.m2:8:10-8:13: --source code:
     b := 1/x;

ii4 : current

oo4 = 2-OP / 1
             fetch 0 0

oo4 : PseudocodeClosure

ii5 : toString current

oo5 = (2-OP / 1 (fetch 0 0))

ii6 : disassemble current

oo6 = (2-OP, /, {1, fetch 0 0})

oo6 : Sequence

ii11 : class \ flatten oo6

oo11 = (String, SymbolBody, Pseudocode, Pseudocode)

oo11 : Sequence

You said it's for internal use, but I don't understand what changes if everything was nested sequences without any Pseudocode. Colloquially if someone asked me for to disassemble a compiled binary code I wouldn't show them a file with some C written in there, it would be all assembly.

None of these changes have anything to do with how the interpreter works.

I know you're not changing how the interpreter works. The point of disassemble was to translate a piece of code to the lowest level that the interpreter understands, which for M2 is lisp-like. It's not meant to be particularly human-readable or efficient, but unambiguous and simple to parse. Introducing new human-readable format for Pseudocode is useful and appreciated, but it's not the same thing.

For that reason, I think disassemble should be exported and we should still show its output in the documentation in addition to the pseudocode output, because different people (e.g. you and I) prefer different outputs and there's no reason to hide one in Core. If that's too much work, I can also export and document it myself after this is merged.

[pzinn]

CodeSequence and CodeList are just for output purposes, they have no particular meaning. Clearly you have a different idea than I what the "purpose" of disassemble is. If so, perhaps you should write your own disassemble routine.

[mahrud]

Yes, that's true, but the idea and purpose I have in mind for disassemble is the existing one, as documented here, which this PR takes away. Maybe the internal-only tool you call disassemble should have a different name and you should define disassemble Pseudocode := toString, and leave its documentation intact? If you don't even export the function you have implemented, I don't know why implementing your proposal depends on removing disassemble. I genuinely don't understand why you say this is "not the way to go".

[pzinn]

Fine, I can rename disassemble something else and have disassemble point to toString. For the rest, if you have a precise example where you think the parenthesising can be improved, let me know.

[pzinn]

instead of disassemble, toList is used to turn Pseudocode into a list that's amenable to printing / using _. disassemble is exported and acts as toString on Pseudocode.

[d-torrance]

I think I actually kind of miss the colons...

Previously, open parentheses indicated the start of a new pseudocode object:

i1 : disassemble(() -> if true then 5)

o1 = (function restargs: false numparms: 0 framesize: 0 frameID: 8610 (if
     (global-fetch 218) then: 5 else: (null)))

But now, there are lots more opening parentheses:

i1 : disassemble pseudocode(() -> if true then 5)

o1 = (function (restargs false) (numparms 0) (framesize 0) (frameID 10541) (body
     (if (global-fetch 219) (then 5) (else null))))

It looks like maybe restargs, numparams, etc, might indicating new pseudocode objects, but they're really just members of the function struct.

Here are a couple possible lisp-y alternatives:

• Dotted pair notation:

  (function (restargs . false) (numparms . 0) (framesize . 0) (frameID . 10541) (body .
  (if (global-fetch 219) (then . 5) (else . null))))

• Leading colons, like lisp keyword symbols:

  (function :restargs false :numparms 0 :framesize 0 :frameID 10541 :body
  (if (global-fetch 219) :then 5 :else null)))

[d-torrance]

Also, what's the difference between Pseudocode and PseudocodeClosure?

[pzinn]

You guys make up your mind which notation you want for toString, I don't particularly care. For the record the documentation says nothing about which particular form of parenthesisation disassemble should produce.

[pzinn]

it used to be that Pseudocode was the m2 equivalent of the d class CodeClosure. That was not very consistent.
Now CodeClosure -> PseudocodeClosure and Code -> Pseudocode.

[mahrud]

Paul, I'm mostly happy with this now, but the PR is making a lot of changes in d (in particular many changes in the first commit), and I'm nervous about finding a bug in the future and the difficulty of figuring out which changes caused the bug. Do you mind if I make a new PR where I take exactly your changes, but break them up into smaller commits? (All commits still keeping you as the author.) This would make bisecting easier. I can also fix the merge conflicts.

Doug, I hear your point, but I think we can come back and fix the string printing later. Does that sound okay?

Dismissing review because no longer requesting changes, but I'm working on splitting the commits (preserving Paul's authorship) for easier bisecting in the future.

[mahrud]

One thing i hadn't thought of before: moving pseudocode and disassemble out to code.m2 means that if there was a problem earlier, say in startup.m2, or in any of the files that need to be loaded before code.m2, we can't use pseudocode/disassemble to debug it. I think at least one of them needs to be in the interpreter.

[d-torrance]

Doug, I hear your point, but I think we can come back and fix the string printing later. Does that sound okay?

Oh totally -- I'm just splitting hairs.

[mahrud]

it used to be that Pseudocode was the m2 equivalent of the d class CodeClosure. That was not very consistent. Now CodeClosure -> PseudocodeClosure and Code -> Pseudocode.

Should Pseudocode really be PseudocodeBody?

i1 : pseudocode(() -> 1/0)

o1 = function restargs false
              numparms 0
              framesize 0
              frameID 11336
              body 2-OP / 1
                          0

o1 : PseudocodeClosure

i2 : pseudocode functionBody(() -> 1/0)

o2 = 2-OP / 1
            0

o2 : Pseudocode

[pzinn]

the correspondence between d classes and m2 classes is not particularly consistent. FunctionBody in m2 is just functionCode in d (also, annoying lack of capitalisation). so, yes, one could have PseudocodeBody, and then both it and PseudocodeClosure could descent from a Pseudcode which would be the pseudocode analogue of Function. I decided to keep things simpler but it's debatable what is better.

[DanGrayson]

Could you resolve the conflicts, @pzinn ?

[pzinn]

I think @mahrud rewrote this PR -- I will check out the "split" version soon. closing.