Location doesn't handle multiline nodes

[Jozott00]

class A {}

class A {}

class A {
}

This code results in the following error message:

-- CompileError ----------------------------------------------------------------

  4| class A {}
     ^^^^^^^^^^

Class `A` was already defined.
-- CompileError ----------------------------------------------------------------

  6| class A {
     ^

Class `A` was already defined.

As you can see, the first error, the class definition is completely marked (as it should be) since it is a one-liner, but in the second error message only the first letter is marked.

This problem holds for all multiline expressions.

[flofriday]

True this is just a missing feature in CompileError. It already has the full location but does not really know how to render them.

[Jozott00]

Ok nice!

I added some methods to Location and Token, so we can merge via method calls.

someTokenOrLocation.mergeLocations(otherTokenOrLocation)

[flofriday]

There are already merge functions in location.swift. I don't quite get the point of also having methods dangling of the objects.

Either way we sould choose one approach and stick to it. At least there should only be the merging logic once.

On Mon, 12 Sep 2022, 11:50 Johannes Zottele, @.***> wrote:

Ok nice!

I added some methods to Location and Token, so we can merge via method calls.

someTokenOrLocation.mergeLocations(otherTokenOrLocation)

— Reply to this email directly, view it on GitHub https://github.com/flofriday/Moose/issues/36#issuecomment-1243489802, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFBZOL6L63PXTGU4Y42JVZDV534GLANCNFSM6AAAAAAQKJGJWM . You are receiving this because you were assigned.Message ID: @.***>

[Jozott00]

The merge logic is bundled in mergeLocations(_, _) but I find the object oriented approach much cleaner than calling a global function.

[Jozott00]

I found a mistake in the mergeLocation function:

    // Lets assume b is the last one
    var endCol = b.endCol

    // Now check if we made another mistake
    if (a.line > b.line) || (a.line == b.line && a.endLine > b.endLine) {
        endCol = a.endCol
    }

The correct condition would be

if (a.endLine > b.endLine) || (a.endLine == b.endLine && a.endCol > b.endCol) {

But I would suggest that we use take advantage of the swift tuple comparison to implement the whole logic:

    // Lets assume a is the first one
    var col = a.col

    // Now check if we made a mistake and if so lets correct for it
    if (b.line, b.col) < (a.line, a.col) {
        col = b.col
    }

    // Lets assume b is the last one
    var endCol = b.endCol

    // Now check if we made another mistake
    if (b.endLine, b.endCol) < (a.endLine, a.endCol) {
        endCol = a.endCol
    }

Is it ok for you if I use the second implementation? I think it is easier to understand

[Jozott00]

I found a mistake in the mergeLocation function:

    // Lets assume b is the last one
    var endCol = b.endCol

    // Now check if we made another mistake
    if (a.line > b.line) || (a.line == b.line && a.endLine > b.endLine) {
        endCol = a.endCol
    }

The correct condition would be

if (a.endLine > b.endLine) || (a.endLine == b.endLine && a.endCol > b.endCol) {

However I would suggest that we take advantage of the swift tuple comparison to implement the logic:

    // Lets assume a is the first one
    var col = a.col

    // Now check if we made a mistake and if so lets correct for it
    if (b.line, b.col) < (a.line, a.col) {
        col = b.col
    }

    // Lets assume b is the last one
    var endCol = b.endCol

    // Now check if we made another mistake
    if (b.endLine, b.endCol) < (a.endLine, a.endCol) {
        endCol = a.endCol
    }

Is it ok for you if I use the second implementation? I think it is easier to understand

[flofriday]

Sure go ahead, could you also add some unit tests?

[flofriday]

Ok I will work on this now.

[flofriday]

Ok nice!

I added some methods to Location and Token, so we can merge via method calls.

someTokenOrLocation.mergeLocations(otherTokenOrLocation)

I actually think it might be preferable to have many constructors of location which take tokens and other locations and merge them. Since it is a natural way to create new Locations by merging two existing ones.

[flofriday]

Also I now have implemented multiline error highlighting, however I think that this issue should stay open since we should aim to not highlight the whole class but probably only the class keyword and the name:

-- CompileError ----------------------------------------------------------------

   |
  4| class A {}
   | ^^^^^^^

Class `A` was already defined.

This would make the error message much shorter, easier to read without loosing any meaningful information.

[Jozott00]

Yes, this is done by the parser by using not the closing bracket as second location mergepoint but the first one. In any case we could limit the printed code inside the error to one line with an ending ... in case their are more then one line.

[flofriday]

Well I thought that in the typechecker we could create another error method that takes a a location instead of an AST node or token. And in special cases like this we could create a special location of the thing we want to highlight. Like in this case we could merge the class token of the keyword and the name and send this to the error.

[flofriday]

On class redefinition we will now only print the relevant part of the class, and ignore the body since it doesn't matter here.