hoaproject / Compiler

The Hoa\Compiler library.
https://hoa-project.net/
453 stars 47 forks source link

Fix the incorrect display of the column and line in errors. #79

Open SerafimArts opened 6 years ago

SerafimArts commented 6 years ago

Fixes "Incorrect calculation of line and position in utf-8 files" issue

Unrecognized token:

image

Unexpected token:

image

:smile_cat:

Hywan commented 6 years ago

Thanks for the PR!

I would use a custom exception constructor (that can use a trait or a parent class) to compute the correct line, column, marker/arrow position etc. rather than a trait in the Lexer and the Parser classes. It makes more sense for me.

Can you update your patch this way please? If you don't have time, I can do it for you.

SerafimArts commented 6 years ago

@Hywan With such a signature?

$error = SomeErr::createFromOffest(string $message, array $args = [], string $sources, int $offset)
SerafimArts commented 6 years ago

Or maybe:

$message = \sprintf('...', ...$args);

throw SomeErr::fromOffset($message, string $sources, int $offset);
Hywan commented 6 years ago

What about:

class UnrecognizedToken extends Exception
{
    …

    public function __construct($code, $text, $byteOffset)
    {
        // …
        parent::__construct($message, $code, $args);
    }
}

Notice that $message and $args are no longer part of the signature, but they are computed and passed to the parent constructor.

Thoughts?

SerafimArts commented 6 years ago

This will make it impossible to reuse the exception with arguments that are different from those specified in the constructor. And this is a violation of backward compatibility.

Is such an improvement of such changes worthwhile?

Hywan commented 6 years ago

Or add a static method on UnrecognizedToken to build a valid UnrecognizedToken exception with a pre-defined message? Something like:


class UnrecognizedToken extends Exception
{
    public static function blabla(…)
    {
        // $message
        return new self($message, …);
    }
}
SerafimArts commented 6 years ago

This I suggested above. Just now there is no time to realize =\

I think I'll fix it this week.

Hywan commented 6 years ago

Heh, sorry :-).

SerafimArts commented 6 years ago

@Hywan ping?

Pierozi commented 6 years ago

@SerafimArts PR was closed for a reason?

SerafimArts commented 6 years ago

@Pierozi for me it became not relevant. I had to fork the project and slightly rewrite it: https://github.com/railt/compiler/network

Hywan commented 6 years ago

We are still interested by the PR though… Can you reopen it please?

SerafimArts commented 6 years ago

nope =( image

Pierozi commented 6 years ago

you can still get the code of the PR, in order to recreate your branch

$ git clone git@github.com:hoaproject/Compiler.git
$ cd Compiler
$ git fetch origin pull/79/head:issue/78
$ git remote add my-fork ....
$ git push my-fork issue/78
SerafimArts commented 6 years ago

Done

SerafimArts commented 6 years ago

In any case, in this PR I did not take into account the behavior of %pragma lexer.unicode, so it is not entirely correct. More precisely, exceptions always take utf into account, regardless of whether there are instructions to use it.

Hywan commented 6 years ago

Thanks for reopening!