line numbers in the AST re #732 #750

ffrank · 2024-03-23T19:02:02Z

I wrote a suggestion of how to store the mcl code coordinates of notable AST nodes in the parser. (It's not done for each last node.)

The approach is possibly flawed; the amount of copy/paste makes me sad.

This was still the easier part though. Next I was trying to figure out how to generate an error message during type unification, that indicates the == operator in the following snippet:

$a = 1
$b = "one"

if $a == $b {
        $c = 3
}

But the unification approach that collects all invariants, and then loops over them, makes this very hard, maybe impossible.

Will we need to keep references to the AST nodes in the invariants? So that we can trace where problematic invariants originate?

purpleidea · 2024-03-23T19:13:34Z

the amount of copy/paste makes me sad.

I expect some copy-paste, that's to be expected.

during type unification

Don't worry about this at the moment. I have some structural changes which are coming and I don't know if this will break anything that happens there yet, so TBD.

It's not done for each last node.)

Just a POC is fine!

purpleidea · 2024-03-23T19:14:44Z

PS: Note there is also the %error stuff in parser-- that works well enough. Line numbers in the code is the goal =D

purpleidea

Just some random quick thoughts.

lang/parser/parser.y

lang/ast/structs.go

ffrank · 2024-03-24T21:57:10Z

In the latest commit 913c100 the code essentially works. It will store start and end positions in nodes types that have received the TextArea embed.

This does not survive interpolation though, unless Interpolate is adapted as demonstrated for StmtBind and StmtIf here.

I even have an idea for a type unifier that could allow us to drill down to the offensive expressions.

EDIT: after some shy attempts to implement a prototype, I want to say this idea did not pan out. Refactored unification sounds much more promising 👇🏻

purpleidea · 2024-04-01T17:15:34Z

FYI: I'm currently refactoring the type unification stuff, so after that's done it will be great to revisit this and get it merged in some form. Sneak peak: The new unification has a pointer to the Expr in question so as long as the Expr has the ROW/COL information, then we'll be able to show that without doing any additional plumbing.

ffrank · 2024-04-13T14:12:30Z

I'm not moving this in any direction until that change lands then.

purpleidea · 2024-04-13T14:26:19Z

Yeah, sorry for the delay. The start of that is here: https://github.com/purpleidea/mgmt/tree/feat/unification in case you're knowledgeable in this area and want to help. I'm kind of a terrible algorithmist and a few things are up in the air, but if you or anyone is an expert in this area, please chime in =D

purpleidea · 2024-07-01T23:04:38Z

Boy has time flown by! I guess you know all the things I was busy with. In any case, unification has merged, so if you'd like to rebase this, that would be awesome!

It might also be very easy for you to plug things into unification error messages too (or I can help with that)

LMK thanks!

ffrank · 2024-09-08T20:41:17Z

Hey, nice work on the resolver, feels good so far.

I can build this branch and ./mgmt run lang error.mcl where error.mcl has

# this shall fail
$value = 1 + "a"
test "t1" { anotherstr => $value, }

The error message will be this:

cli parse error: could not unify types: unify error with: topLevel(func() { <built-in:_operator> }): type error: int != str

The trouble is that the error is reported at the definition of the + operator rather than the invocation.

Note that I did change the String function for FuncCalls to give the location. But it is not used at the moment. I have a commit in here that makes mgmt dump a string representation of the AST and it includes

call:_operator(str("+"), int(1), str("a")) @ (1 1)

(Never mind that this position is also wrong, but right now it seems more important that the resolver can report it at all.)

ffrank · 2024-09-08T20:53:07Z

(Wrong text position now also fixed)

purpleidea · 2024-09-09T20:40:08Z

resolver

What's a resolver?

The trouble is that the error is reported at the definition of the + operator rather than the invocation.

All type unification errors that are found during unification are reported at the same place... We can likely improve errors a bit though.

You asked me to comment and look at this here, I need a bit more info what you're asking, sorry!

ffrank · 2024-09-10T17:58:19Z

Okay trying to clear it up. I meant solver of course, not resolver.

The current issue is that we get this error:

topLevel(func() { <built-in:_operator> }): type error: int != str

That is correct, but more helpful to the user will be the information that the call of this operator is 1 + "a" in line 2 column 7.

In essence, I see a need to show a "call stack". Of course, this is at type checking time, so nothing is called (is it?)

Maybe what's rather needed is a few levels of AST parents. Rather than just saying "this unnamed internal operator was given bad operands", we should include that "its parent is this expression 1 + "a" in this bind statement $value = ...". (The immediate parent will be enough in this example, but not sure if more complex code will bring a need for more.)

purpleidea · 2024-09-10T18:43:43Z

Okay trying to clear it up. I meant solver of course, not resolver.

Duh now! I don't know why my mind missed that, lol

That is correct, but more helpful to the user will be the information that the call of this operator is 1 + "a" in line 2 column 7.

Yeah I agree, we probably want to hide the "ExprTopLevel" and "ExprSingleton" expressions. To do so, you'd want to wrap this x.Expr call:

mgmt/lang/unification/fastsolver/fastsolver.go

Line 138 in 3d06605

return nil, errwrap.Wrapf(err, "unify error with: %s", x.Expr)

With trueCallee from here:

mgmt/lang/ast/util.go

Line 330 in 3d06605

// trueCallee is a helper function because ExprTopLevel and ExprSingleton are

I expect this will cause an import cycle, so I'd have to ponder what the best move is... I'd like to avoid needing to add a new method on Expr to get the "TrueCallee" for everyone.

Does that help?

ffrank · 2024-09-10T21:08:30Z

Yes the cycle prevented doing it from the solver. Spot on.

I pushed a silly little hack to do this recursion from the GAPI scope, but it does not work. This is the output now:

cli parse error: could not unify types [[ in expression: func() { <built-in:_operator> } ]]: unify error with: topLevel(func() { <built-in:_operator> }): type error: int != str

So the result from trueCallee still does not show information.

This makes sense to me. As said, it's true that it's the + operator that ultimately is affected. But we don't care. We are interested in its AST parent(s).

purpleidea · 2024-09-10T23:02:11Z

You want to unwrap the x.Expr part...

If you see this as your error:

topLevel(func() { built-in:_operator }): type error: int != str

Then TrueCallee(_) would return:

func() { built-in:_operator }: type error: int != str

If that's not what you want, I am lost. But I don't see how this error is not correct. What do you expect it to print?

ffrank · 2024-09-11T10:42:10Z

I may be barking up the wrong tree, going on about the AST.

But the issue (in terms of AST nodes) is that this type error is reported on the ExprFunc (i.e. the declaration) of the + operator, which will never be helfpul. I need it reported on the ExprCall representing (in this case) 1 + "a".

Remember, ultimately we want to report the line and column number of the code that caused the unification issue.

purpleidea · 2024-09-11T16:14:05Z

the issue (in terms of AST nodes) is that this type error is reported on the ExprFunc (i.e. the declaration) of the + operator, which will never be helfpul. I need it reported on the ExprCall representing

I agree. I don't know the correct way to achieve this at the moment, but I will dig into it if I can.

Remember, ultimately we want to report the line and column number of the code that caused the unification issue.

If it reports the definition site instead of the call site for now, that's okay =D

ffrank · 2024-09-12T12:28:10Z

If it reports the definition site instead of the call site for now, that's okay =D

I don't believe I agree, at least until I see a typing issue that is not reported on this kind of builtin element. Why have line numbers when we cannot report errors that use them?

I had the idea to include the pointer to the entire AST node in the error, rather than just the Expr within. Then, we could run a graph search through the AST, so that we can basically "zoom in" on the problematic expression(s).

The problem is that I cannot very easily run a search through the AST. That's why I created #773 in order to have some discussion around that. (It might be good to talk about this in person a little.)

ffrank · 2024-09-22T21:45:10Z

ping @purpleidea

purpleidea · 2024-09-23T17:12:26Z

Just had a look. TBQH, I'm not the expert here, so I don't know how to proceed, sorry.

purpleidea · 2024-09-28T15:33:50Z

Maybe if we did a pair programming we might be able to come up with some ideas? LMK

ffrank · 2024-10-21T12:06:00Z

I'm traveling atm but maybe we can make it happen in November.

purpleidea · 2024-10-21T14:21:32Z

Sure LMK when ready.

ffrank · 2024-12-26T13:56:53Z

First complete PoC implementation.

$ ./mgmt run lang examples/lang-errors/simple_types.mcl 
This is: mgmt, version: 0.0.26-206-g1b0b92f2
Copyright (C) 2013-2024+ James Shubin and the project contributors
Written by James Shubin <[email protected]> and the project contributors
14:53:14 main: start: 1735221194038676643
14:53:14 cli: lang: lexing/parsing...
14:53:14 cli: lang: import: /home/felix/mgmt/examples/lang-errors/
14:53:14 cli: lang: running type unification...
14:53:14 cli: lang: possible type issue found at line 4 column 3
14:53:14 main: goodbye!
cli parse error: could not unify types: unify error with: topLevel(func() { <built-in:_operator> }): type error: str != int

(note the final log message before the goodbye)

This breaks some very brittle unit tests. I did not touch those yet, it might be worth refactoring them rather than doing the trivial fix.

One big missing piece is that we cannot easily determine file names (i.e., from which file was a given parser token read), so in a complex code base with multiple mcl files, the message will still be quite ambiguous.

purpleidea

Hey @ffrank HNY!

This looks really promising. Have a look at some of my comments if you can, I had a few questions, and a likely answer for the filename bit.

But I think I could merge this fairly soon...

I do have a few nit's but I can fix those up unless you have any objections to that.

Cheers!

purpleidea · 2025-01-02T20:55:40Z

lang/parser/parser.y

@@ -221,7 +221,7 @@ stmt:
 	// `func name(<arg>, <arg>) { <expr> }`
 |	FUNC_IDENTIFIER IDENTIFIER OPEN_PAREN args CLOSE_PAREN OPEN_CURLY expr CLOSE_CURLY
 	{
-		posLast(yylex, yyDollar) // our pos


qq: Why are some of the changes for posLast => locate done in the same space, and others done after the $$... definition bit? Does it matter? (See other comment for an example.)

purpleidea · 2025-01-02T20:56:04Z

lang/parser/parser.y

 		$$.stmt = $1.stmt
+		locate(yylex, $1, yyDollar[len(yyDollar)-1], $$.stmt)


Here's an example of one that moves to after the $$. = bit...

purpleidea · 2025-01-02T20:56:39Z

lang/parser/parser.y

 	}
 |	IF expr OPEN_CURLY prog CLOSE_CURLY ELSE OPEN_CURLY prog CLOSE_CURLY
 	{
-		posLast(yylex, yyDollar) // our pos
+		locate(yylex, $1, yyDollar[len(yyDollar)-1], $$.stmt)


Here's an example where it stays in the same place.

purpleidea · 2025-01-02T21:00:41Z

lang/ast/util.go

@@ -393,3 +393,23 @@ func lambdaScopeFeedback(scope *interfaces.Scope, logf func(format string, v ...
 		logf("$%s(...)", name)
 	}
 }
+
+func AreaParentOf(needle, haystack interfaces.Node) LocalNode {


Some explainer comment doc would be very helpful here please.

purpleidea · 2025-01-02T21:02:18Z

lang/interfaces/unification.go

 	// Expect is one of the two types to unify.
 	Expect *types.Type

 	// Actual is one of the two types to unify.
 	Actual *types.Type
+
+	// An error string to pass along with this


Cute idea to make this implement the error interface... But why do we want to do this? Do we want to change the signature somewhere to just include a Node or error context struct ?

purpleidea · 2025-01-02T21:18:39Z

lang/gapi/gapi.go

+			cause := unifyErr.(*interfaces.UnificationInvariant).Node
+			parent := ast.AreaParentOf(cause, iast)
+			line, col := parent.GetPosition()
+			logf("possible type issue found at line %d column %d", line, col)


On each Node, we can get the filename from obj.data.Metadata.Main IIRC. And the path of that from obj.data.Base again IIRC.

If that doesn't work, I can surely fix it though.

ffrank mentioned this pull request Mar 23, 2024

Dig into line/row numbers in the parser #732

Open

purpleidea reviewed Mar 23, 2024

View reviewed changes

lang/parser/parser.y Outdated Show resolved Hide resolved

lang/parser/parser.y Show resolved Hide resolved

lang/ast/structs.go Outdated Show resolved Hide resolved

ffrank force-pushed the linenumbers branch from 913c100 to c37ab56 Compare September 8, 2024 20:33

purpleidea force-pushed the master branch from 2431df9 to 71a1efd Compare October 30, 2024 04:41

purpleidea force-pushed the master branch 3 times, most recently from b8072b2 to 380004b Compare November 23, 2024 06:25

cli: run: Note an apparent UX issue

5768f0b

ffrank and others added 6 commits December 19, 2024 22:44

lang: parser: Simple WiP implementation of code positions in AST nodes

bfd832c

lang: parser: Store locations in various AST nodes

fa786fa

lang: ast: Fix up Interpolate() for ExprCall

408ee8c

lang: gapi: Identify the AST node that caused unification to fail

de77311

lang: Add examples with errors for testing

96cf458

lang: Fix formatting

1b0b92f

ffrank force-pushed the linenumbers branch from ebb8255 to 1b0b92f Compare December 26, 2024 13:24

purpleidea reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

line numbers in the AST re #732 #750

line numbers in the AST re #732 #750

ffrank commented Mar 23, 2024

purpleidea commented Mar 23, 2024

purpleidea commented Mar 23, 2024

purpleidea left a comment

ffrank commented Mar 24, 2024 •

edited

Loading

purpleidea commented Apr 1, 2024

ffrank commented Apr 13, 2024

purpleidea commented Apr 13, 2024

purpleidea commented Jul 1, 2024

ffrank commented Sep 8, 2024 •

edited

Loading

ffrank commented Sep 8, 2024

purpleidea commented Sep 9, 2024

ffrank commented Sep 10, 2024 •

edited

Loading

purpleidea commented Sep 10, 2024

ffrank commented Sep 10, 2024

purpleidea commented Sep 10, 2024

ffrank commented Sep 11, 2024

purpleidea commented Sep 11, 2024

ffrank commented Sep 12, 2024

ffrank commented Sep 22, 2024

purpleidea commented Sep 23, 2024

purpleidea commented Sep 28, 2024

ffrank commented Oct 21, 2024

purpleidea commented Oct 21, 2024

ffrank commented Dec 26, 2024

purpleidea left a comment

purpleidea Jan 2, 2025

purpleidea Jan 2, 2025

purpleidea Jan 2, 2025

purpleidea Jan 2, 2025

purpleidea Jan 2, 2025

purpleidea Jan 2, 2025

		$$.stmt = $1.stmt
		locate(yylex, $1, yyDollar[len(yyDollar)-1], $$.stmt)

line numbers in the AST re #732 #750

Are you sure you want to change the base?

line numbers in the AST re #732 #750

Conversation

ffrank commented Mar 23, 2024

purpleidea commented Mar 23, 2024

purpleidea commented Mar 23, 2024

purpleidea left a comment

Choose a reason for hiding this comment

ffrank commented Mar 24, 2024 • edited Loading

purpleidea commented Apr 1, 2024

ffrank commented Apr 13, 2024

purpleidea commented Apr 13, 2024

purpleidea commented Jul 1, 2024

ffrank commented Sep 8, 2024 • edited Loading

ffrank commented Sep 8, 2024

purpleidea commented Sep 9, 2024

ffrank commented Sep 10, 2024 • edited Loading

purpleidea commented Sep 10, 2024

ffrank commented Sep 10, 2024

purpleidea commented Sep 10, 2024

ffrank commented Sep 11, 2024

purpleidea commented Sep 11, 2024

ffrank commented Sep 12, 2024

ffrank commented Sep 22, 2024

purpleidea commented Sep 23, 2024

purpleidea commented Sep 28, 2024

ffrank commented Oct 21, 2024

purpleidea commented Oct 21, 2024

ffrank commented Dec 26, 2024

purpleidea left a comment

Choose a reason for hiding this comment

purpleidea Jan 2, 2025

Choose a reason for hiding this comment

purpleidea Jan 2, 2025

Choose a reason for hiding this comment

purpleidea Jan 2, 2025

Choose a reason for hiding this comment

purpleidea Jan 2, 2025

Choose a reason for hiding this comment

purpleidea Jan 2, 2025

Choose a reason for hiding this comment

purpleidea Jan 2, 2025

Choose a reason for hiding this comment

ffrank commented Mar 24, 2024 •

edited

Loading

ffrank commented Sep 8, 2024 •

edited

Loading

ffrank commented Sep 10, 2024 •

edited

Loading