Fix to #225, added better error reporting for internal compiler errors.

[srijan-paul]

Previously, when an incorrect table was passed into a function dealing with a certain type of table, only an "impossible" error was thrown followed by a Lua stack trace.

Now it's possible to see the tag which caused the error, and wherever appropriate, an error message following it. Calls to error("impossible") have been replaced with a call to typedecl.tag_error(tag, "(optional error message.)").

Hopefully, this should fix #225 .

[hugomg]

Lovely! Thanks for going through this. I think you even found some cases that I would have missed if I tried doing this myself.

I do have some suggestion for changes before we can merge this though.

The first is that if we choose a good default error message for tag_error then I think we should be able to use the default error message in more places. I think it would be good if we had a clear rule for whether we should have a special error message or not. That way, we don't need to decide for every possible if-elseif whether it should have a special error message or not. One rule of thumb we could follow is that if the if-elseif covers all the possible cases then it can use the default error message and if it only covers a subset of the possible cases then it should have a special error message.

Another thing that we could change is that there are some places in the code that could be updated to use the new tag_matches function. You can find them by running grep -R 'string.match(.*tag'.

[srijan-paul]

I made the changes as asked, excuse the CI fails in older commits, I made some mistakes by merging into master too early. Everything should now work as expected.

One part I couldn't figure out is this line, in coder.lua:1508 :

local name = assert(string.match(cmd._tag, "^ir%.Cmd%.(.*)$"))

I had originally replaced that with this:

local name = cmd._tag
assert(typedecl.tag_matches(name, "ir.Cmd."))

however, doing so would cause the tests to fail, and I seem to be unable to diagnose why.
So I left that one line unchanged. It would be awesome if you could explain that. cheers.

[hugomg]

string.match(cmd._tag, "^ir%.Cmd%.(.*)$")
however, doing so would cause the tests to fail, and I seem to be unable to diagnose why.

The string.match() here is using a string capture. For example, if cmd._tag is ir.Cmd.Binop then name will be Binop.

Maybe we should change the tag_matches function to return this string instead of just true/false?

[hugomg]

Thanks. I marked all the things that you fixed as resolved. There are only a handful that were missed that are still there:

• The tag_error for unknown unary operators
• There is stll an assert(string.match(typ._tag, "^types%.T%.")) in types.lua
• There is the name = assert(string.match(cmd._tag, "^ir%.Cmd%.(.*)$")) that broke the CI

[hugomg]

About the broken CI, if we really wanted we could use git rebase -i to fix the old commits so they pass the CI. However, in this case it is only a very small problem so I don't think we have to bother with it.

[srijan-paul]

The string.match() here is using a string capture. For example, if cmd._tag is ir.Cmd.Binop then name will be Binop.

Maybe we should change the tag_matches function to return this string instead of just true/false?

Yeah that would seem like a good idea.
I missed the capture group, kinda rusty with regex. Thanks for explaining.

I feel like the name tag_matches implies a boolean. What would you say about letting that one line stay as is instead of changing intent of the function for just one usage ?

[srijan-paul]

Good idea. It would be safer to always add the second .. What do you think about adding this . inside the tag_matches function, so the caller doesn't have to remember to add it themselves?

Since the tag_matches function does prefix matching, I personally would find it easier if it only matched against the string it receives as an argument instead of adding a ".". It could, at times cause confusion. Thoughts ?

[hugomg]

I personally would find it easier if it only matched against the string it receives as an argument instead of adding a ".".

Maybe it should have a different name instead of tag_matches then? What we we are trying to see is if a certain tag (such as "types.T.Boolean") has a given type (such as types.T). We aren't trying to match against an arbitrary string...

I feel like the name tag_matches implies a boolean.

If you can think of a good name that doesn't imply a boolean we could use that. If not, I think it is OK if we continue using string.match("ir%.Cmd%.(.*)") in that one place.

[srijan-paul]

If you can think of a good name that doesn't imply a boolean we could use that.

I went with match_tag and made the changes as asked.
The function now adds a second "." and captures and returns everything following it.

Probably not the best name but this is loosely similar to what string.match does in Lua , or Javascript,
match a pattern in a string and return the matched sequence. Except this returns the part following the ..

Do let me know if I missed something yet again :D

[hugomg]

Nice! I like the new name.

I found a couple of small bugs but they should be easy to fix and after that I think this should be good for merging.

It would also be nice if we added some test cases to typedecl_spec.lua. In particular, we should test that something like typedecl.match_tag("abc.d.e", "a.c") returns false because that is a bug that already hit us before and it happened again today. Would you be able to help by writing those tests?

[hugomg]

Should this be local tagname = assert(typedecl.match_tag(cmd._tag, "ir.Cmd"))?

[srijan-paul]

Oh yes, my bad.

[srijan-paul]

It would also be nice if we added some test cases to typedecl_spec.lua. In particular, we should test that something like typedecl.match_tag("abc.d.e", "a.c") returns false

Yeah, I should be able to put together some simple tests for that.

[srijan-paul]

I updated that assert guard, changed match_tag to no longer treat it's arguments as regex strings.
Also added 3 basic test cases to typedecl_spec.lua.

So, do you think these test cases are good for now ?

[hugomg]

Just some small things in the typedecl_spec and then I think this is done.

[hugomg]

• We could put each test case in a separate it block. That way we can give a descriptive name for each of them. For example the first test can say that it is testing that the "." is not being interpreted as a regex character.
• The second test case could test that the return value is "baz" instead of only testing that it is truthy
• I think we don't need to put that "") at the end

[srijan-paul]

True, should have gone with more descriptive test logs.
Done 👍.

[hugomg]

Perfect! Thanks a lot for the contribution! It's much appreciated.