Improve the error reporting in meson #12154

alexandre-janniaux · 2023-08-24T09:38:38Z

Hi,

~~This MR is in a RFC state for now, many things need to be improved before merging,~~ but I'd like to have feedback on how the error is improved and how to handle the line reporting itself before.

This MR stems from a very simple case where I found the error reporting very confusing with the following code:

if host_system = 'android'

When running meson, we would only get the following confusing error:

meson.build:6:17: ERROR: Cannot perform 'if' operation on void statement.

This MR thrive to add the necessary infrastructure and convention to report better errors with a way to solve the issue. Typically, the previous example would now output the following error:

meson.build:6:3: ERROR: Assignment are returning void and not valid in if clause.
meson.build:6:3:  | if host_system = 'android'
meson.build:6:3:  |    ^^^^^^^^^^^^^^
meson.build:6:3:  |    Did you forget a = here?

The current implementation is conservative to where the errors were not extended to this new information.

Many questions are arising from that regarding the formatting:

Should we append meson.build:6:3: in front of every line or lighten the presentation?
Should we append the line number on location where there is code?
How should we handle multi-line errors?
Should we test those errors and the span (which looks wrong here) in the CI?

I also implemented the line reporting in an ugly way, by storing the relevant line in every (well for now, only IfClauseNode and AssignmentNode, but that's one of the hypothesis of the code) AST node, but some nodes are spanning on multiple lines and there can be multiple nodes per line, so it looks a bit ugly even though we can make python use a single string in the back. We can also only report the relevant part of the code by referencing self.code[] or referencing the relevant substring when raise is happening. What would you prefer?

xclaesse · 2023-08-24T13:03:32Z

Better error reporting is always great.

Played a bit with wrong statements in if condition, the problem is not limited to assignment:

if message('hello') -> ERROR: Cannot perform 'if' operation on void statement..
if fs.copyfile('foo.c') -> ERROR: Object <CustomTargetHolder foo.c@cus: ['/home/xclaesse/.local/bin/meson', '--internal', 'copy', '@INPUT@', '@OUTPUT@']> of type CustomTarget does not support the bool() operator.

alexandre-janniaux · 2023-08-24T13:12:46Z

Played a bit with wrong statements in if condition, the problem is not limited to assignment:

Of course yes, as of now this is just a PoC, I'd like to improve where the error marker lands also so that it's easier to track with the eyes, and improve the line reporting since every node would need to include that, which is a bit ugly as-is.

Thanks a lot for some additional checks! That's very interesting given we'll typically need to unwind back in the evaluation for the fs.copyfile back to improve the reporting. Would you like to improve the errors after this message yourself or should I include that in the MR?

tristan957 · 2023-08-25T17:34:52Z

I like this idea. Developers love when tooling is very explicit and makes their live easier, which this does. Thanks for taking this on. FYI you can mark PRs as drafts too.

I recently started learning Typst, and the compiler is amazing with errors.

tristan957 · 2023-09-06T18:33:59Z

mesonbuild/utils/core.py


    @classmethod
-    def from_node(cls, *args: object, node: BaseNode) -> MesonException:
+    def from_node(cls, *args: object, node: BaseNode, error_resolve: str = None) -> MesonException:


This should be a T.Optional[str]

mesonbuild/mlog.py

tristan957 · 2023-09-06T18:35:18Z

mesonbuild/interpreterbase/interpreterbase.py

+                raise InvalidCode.from_node(
+                    f'Assignment are returning void and not valid in if clause.',
+                    node=i.condition,
+                    error_resolve=f'Did you forget a = here?')


Maybe a better message would be "Should this be an equality check?"

tristan957 · 2023-09-06T18:36:20Z

mesonbuild/mlog.py

@@ -407,8 +409,13 @@ def exception(self, e: Exception, prefix: T.Optional[AnsiDecorator] = None) -> N
                node_resolve.append(empty_lineno + ' | ' + e.colno * ' ')
                node_resolve.append(yellow(f'{e.error_resolve}'))

+            if getattr(e, 'end_lineno', None) is not None and e.lineno != e.end_lineno:
+                node_after += [[ ' ...located before...']]
+                node_after += [ [f' {str(i + e.lineno + 1).rjust(lineno_size)} | ' + line]


Remove the space around the brackets like in [ [.

tristan957 · 2023-09-06T18:36:54Z

mesonbuild/mlog.py

        with self.force_logging():
-            for args_list in [args, node_content, node_marker, node_resolve]:
+            for args_list in [args, node_content, node_marker, node_resolve] + node_after:


Instead of a +, add node_after to the array with node_resolve, *node_after]

tristan957 · 2023-09-06T19:24:16Z

All the linting jobs are failing

alexandre-janniaux · 2023-10-08T21:17:46Z

I've addressed the issues from the two linters, but there are still issues with mypy, mostly because of kwargs. I've made the necessary changes to make it work on recent python, but I'm not sure of what should be done. The remaining issues are because the needed type annotations are not available:

mesonbuild/utils/core.py:39:32: error: Name "T.TypedDict" is not defined  [name-defined]
mesonbuild/utils/core.py:40:11: error: Name "T.NotRequired" is not defined  [name-defined]
mesonbuild/utils/core.py:41:13: error: Name "T.NotRequired" is not defined  [name-defined]
mesonbuild/utils/core.py:42:12: error: Name "T.NotRequired" is not defined  [name-defined]
mesonbuild/utils/core.py:54: error: Name "T.Unpack" is not defined  [name-defined]
mesonbuild/interpreterbase/interpreterbase.py:76: error: Name "T.Unpack" is not defined  [name-defined]

Should I try to get rid of the necessity for kwargs?

tristan957 · 2023-10-09T14:59:02Z

cc @dcbaker

mesonbuild/mlog.py

alexandre-janniaux · 2023-10-10T07:01:53Z

For now, I've switched the **kwargs to T.Any for now, I'll wait for python 3.12 to improve the parameter typing for the exceptions.

I've also fixed the lint errors, and changed the error reporting for void function inside if conditions so that it works with every void function, but now they will be evaluated before. (Unfortunately, I don't have any other solution to get the type for now).

I have a few changes on commit message to make and the mlog patches to re-improve, and an issue with the project with assignment where the carets are not displayed correctly, and this should be good to go.

mesonbuild/mlog.py

tristan957 · 2024-03-16T16:38:31Z

Test failures.

alexandre-janniaux · 2024-03-18T07:22:19Z

Unfortunately, I'm not sure to understand the remaining errors:

CI:

=================================== FAILURES ===================================
___________ LinuxlikeTests.test_generate_gir_with_address_sanitizer ____________
[gw2] linux -- Python 3.11.6 /usr/bin/python3

self = <unittests.linuxliketests.LinuxlikeTests testMethod=test_generate_gir_with_address_sanitizer>

    @skip_if_not_base_option('b_sanitize')
    def test_generate_gir_with_address_sanitizer(self):
        if is_cygwin():
            raise SkipTest('asan not available on Cygwin')
        if is_openbsd():
            raise SkipTest('-fsanitize=address is not supported on OpenBSD')
    
        testdir = os.path.join(self.framework_test_dir, '7 gnome')
        self.init(testdir, extra_args=['-Db_sanitize=address', '-Db_lundef=false'])
>       self.build()

unittests/linuxliketests.py:318: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python3.11/subprocess.py:550: in run
    stdout, stderr = process.communicate(input, timeout=timeout)
/usr/lib/python3.11/subprocess.py:1209: in communicate
    stdout, stderr = self._communicate(input, endtime, timeout)
/usr/lib/python3.11/subprocess.py:2109: in _communicate
    self._check_timeout(endtime, orig_timeout, stdout, stderr)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Popen: returncode: -9 args: ['/usr/bin/ninja', '-w', 'dupbuild=err', '-d', ...>
endtime = 865.065316947, orig_timeout = 300
stdout_seq = [b"ninja explain: deps for 'resources/simple-resources-test.p/meson-generated_.._simple-resources.c.o' are missing\nni...rated-resources-test.p/generated-main.c.o is dirty\nninja explain: resources/generated-resources-test is dirty\n", ...]
stderr_seq = None, skip_check_and_raise = False

    def _check_timeout(self, endtime, orig_timeout, stdout_seq, stderr_seq,
                       skip_check_and_raise=False):
        """Convenience for checking if a timeout has expired."""
        if endtime is None:
            return
        if skip_check_and_raise or _time() > endtime:
>           raise TimeoutExpired(
                    self.args, orig_timeout,
                    output=b''.join(stdout_seq) if stdout_seq else None,
                    stderr=b''.join(stderr_seq) if stderr_seq else None)
E           subprocess.TimeoutExpired: Command '['/usr/bin/ninja', '-w', 'dupbuild=err', '-d', 'explain']' timed out after 300 seconds

/usr/lib/python3.11/subprocess.py:1253: TimeoutExpired
...
FAILED unittests/linuxliketests.py::LinuxlikeTests::test_generate_gir_with_address_sanitizer - subprocess.TimeoutExpired: Command '['/usr/bin/ninja', '-w', 'dupbuild=err', '-d', 'explain']' timed out after 300 seconds

Local:

╰─$ python run_unittests.py -v LinuxlikeTests.test_generate_gir_with_address_sanitizer
 ____________

Meson build system 1.4.99 Unit Tests
================================================================================================ test session starts ================================================================================================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/alexandre/workspace/videolabs/meson
configfile: setup.cfg
plugins: xdist-3.5.0
collected 514 items / 513 deselected / 1 selected                                                                                                                                                                   

unittests/linuxliketests.py::LinuxlikeTests::test_generate_gir_with_address_sanitizer PASSED                                                                                                                  [100%]

================================================================================================= warnings summary ==================================================================================================
unittests/linuxliketests.py::LinuxlikeTests::test_generate_gir_with_address_sanitizer
unittests/linuxliketests.py::LinuxlikeTests::test_generate_gir_with_address_sanitizer
unittests/linuxliketests.py::LinuxlikeTests::test_generate_gir_with_address_sanitizer
  /home/alexandre/workspace/videolabs/meson/mesonbuild/utils/universal.py:1507: EncodingWarning: UTF-8 Mode affects locale.getpreferredencoding(). Consider locale.getencoding() instead.
    encoding = locale.getpreferredencoding()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================================================================== 1 passed, 513 deselected, 3 warnings in 4.96s ===================================================================================
Total time: 5.199 seconds
zsh: command not found: ____________

Is it really related to my changes?

The current error reporting in meson is very poor and only offers the line where the error happened. For instance, with the following code: if host_system = 'android' Then we get the following confusing error: meson.build:6:17: ERROR: Cannot perform 'if' operation on void statement. This commit adds more information inside the MesonException that will be forwarded from the parser and interpreter to also report the line where the error happened, and a potential solution for resolving the error to the meson user. It also adds typing for the internal variables from the MesonException class.

As highlighted from previous commit, the current error reporting in meson has many cases where it will not really help the user find where the error is. In particular, assignment are void expression and in the case where we forget an equal since in a conditional like the follwoing code: if host_system = 'android' Then we only get the following confusing error: meson.build:6:17: ERROR: Cannot perform 'if' operation on void statement. This commit separate this case to more easily notify the user of where the error really happened, and how to solve it. After this commit, the error will now look like: meson.build:6:3: ERROR: Assignment are returning void and not valid in if clause. meson.build:6:3: | if host_system = 'android' meson.build:6:3: | ^^^^^^^^^^^^^^ meson.build:6:3: | Did you forget a = here?

Longer source reference will be expanded below the diagnostic message: meson.build:6:3: ERROR: message(...) returns void and is not valid in condition. meson.build:6:3: 6 | if message('''foo meson.build:6:3: | ^^^^^^^^^^^^^^ meson.build:6:3: | Move the call to message before the condition meson.build:6:3: ...located before... meson.build:6:3: 7 | bar meson.build:6:3: 8 | baz''') There can be errors with lineno > len(line) + 1. See test in test cases/failing/121 missing compiler/.

In a code with: if message(...): The following error was reported: meson.build:6:3: ERROR: Cannot perform 'if' operation on void statement. Now it will be reported as: meson.build:6:3: ERROR: message(...) returns void and is not valid in condition. meson.build:6:3: 6 | if message('''foo meson.build:6:3: | ^^^^^^^^^^^^^^ meson.build:6:3: | Move the call to message(...) before the condition meson.build:6:3: ...located before... meson.build:6:3: 7 | bar meson.build:6:3: 8 | baz''')

MesonException is the first base class which can store sources, lineno, and other AST information to report the error correctly. This removes the need to disable the type checker on the assignments. The Exception type is still kept but is directly thrown without adding additional information.

Setting end_colno will allow to correctly indicate the range of any error involving the node. Since any return/whitespace would break the token into another one, it's safe to assume everything is on a single line, and thus bytespan can be used to directly infer the length in bytes and use this value to compute the end_colno value.

This reverts commit 554be659e7f362ca237b64ce507d039b28d6b28a.

This reverts commit 6be0f9fe1ad88e3748b286432ec7609b268e505c.

Even though the current code uses correct lineno/colno, .from_node() has the benefit of forwarding more information (including end_lineno and end_colno) that might benefit future error reporting even more.

This reverts commit 9a714659ff4abe18a323156fcb7a2534e6c3f556.

Fail after mparser: add resolver string for UTF-8 with BOM case Now, the meson setup will fail (as expected) with the following error message: meson.build:1:0: ERROR: Builder file must be encoded in UTF-8 (with no BOM) meson.build:1:0: 1 | project('utf8 with bom') meson.build:1:0: | ^ meson.build:1:0: | Remove the (invisible) startup BOM character [2] WIP: remove text=

Fail after mparser: exception trigger source and filename addition

tristan957 · 2024-08-07T22:38:03Z

Looks like tests are still failing. A cleaned up git history would also be appreciated!

alexandre-janniaux · 2024-08-08T07:36:20Z

Looks like tests are still failing. A cleaned up git history would also be appreciated!

Indeed, I've pushed to synchronize between my two machines but the MR is back to not being 100% ready. I've added error handling in mparser.py in this MR because of test that were now failing (from the Failing/ directory, but still failing because they were not matching the expected output).

It led to a few fixes on the original MR though, which is nice. I've also changed the behaviour of the caret addition to default to one caret, as it hugely simplify when only a single caret needs to be added and avoid underlining the whole line if the end column is not yet specified.

I'm still working on the intermediate patches so that it doesn't break the code in the middle of the branch. Thanks for checking the MR! I'll post again when this is finished.

alexandre-janniaux requested a review from jpakkane as a code owner August 24, 2023 09:38

alexandre-janniaux force-pushed the better-error/1 branch from 9246fe4 to 41b1b0a Compare September 6, 2023 18:29

tristan957 reviewed Sep 6, 2023

View reviewed changes

github-advanced-security bot found potential problems Sep 6, 2023

View reviewed changes

mesonbuild/mlog.py Fixed Show fixed Hide fixed

tristan957 reviewed Sep 6, 2023

View reviewed changes

alexandre-janniaux changed the title ~~WIP: Improve the error reporting in meson~~ Improve the error reporting in meson Sep 6, 2023

alexandre-janniaux force-pushed the better-error/1 branch from 41b1b0a to af9ab7a Compare September 6, 2023 19:18

alexandre-janniaux force-pushed the better-error/1 branch from af9ab7a to 805a19f Compare September 7, 2023 15:08

alexandre-janniaux force-pushed the better-error/1 branch 2 times, most recently from 727b118 to 208aba0 Compare October 8, 2023 21:15

alexandre-janniaux requested a review from mensinda as a code owner October 8, 2023 21:15

alexandre-janniaux force-pushed the better-error/1 branch from 208aba0 to f65b66e Compare October 9, 2023 09:15

alexandre-janniaux force-pushed the better-error/1 branch from f65b66e to b94210d Compare October 10, 2023 06:54

github-advanced-security bot found potential problems Oct 10, 2023

View reviewed changes

mesonbuild/mlog.py Fixed Show fixed Hide fixed

alexandre-janniaux force-pushed the better-error/1 branch from b94210d to f797e8a Compare January 22, 2024 17:20

alexandre-janniaux force-pushed the better-error/1 branch from f797e8a to f8f2beb Compare March 16, 2024 09:52

github-advanced-security bot found potential problems Mar 16, 2024

View reviewed changes

mesonbuild/mlog.py Fixed Show fixed Hide fixed

mesonbuild/mlog.py Fixed Show fixed Hide fixed

mesonbuild/mlog.py Fixed Show fixed Hide fixed

mesonbuild/mlog.py Fixed Show fixed Hide fixed

alexandre-janniaux force-pushed the better-error/1 branch from f8f2beb to 5000436 Compare March 16, 2024 12:37

github-advanced-security bot found potential problems Mar 16, 2024

View reviewed changes

mesonbuild/mlog.py Fixed Show fixed Hide fixed

alexandre-janniaux force-pushed the better-error/1 branch 2 times, most recently from 95e057b to 8a53bb0 Compare March 16, 2024 15:03

alexandre-janniaux added 6 commits August 5, 2024 23:13

mparser: use kwargs for ParseException

44994f6

alexandre-janniaux force-pushed the better-error/1 branch from fb4ac13 to 9a67adf Compare August 5, 2024 22:09

alexandre-janniaux added 20 commits August 6, 2024 13:13

mparser: switch to kwargs class

0454136

mlog: default to a single caret

998f3fe

WIP: mlog fix col

9efc9e4

WIP: mparser quote

eadce18

[1] mparser: replace plusassignment case

b95f35c

[2] mparser: replace plusassignment line=

5cac985

TODO: mparser: remove useless line= assignment

233d92b

mparser: Function call must be applied to plain id

a34c95e

Revert "TODO: mparser: remove useless line= assignment"

7d46616

This reverts commit 554be659e7f362ca237b64ce507d039b28d6b28a.

Revert "[2] mparser: replace plusassignment line="

ed868bf

This reverts commit 6be0f9fe1ad88e3748b286432ec7609b268e505c.

[1] mparser: update errors to use .from_node()

2ad3b9e

Even though the current code uses correct lineno/colno, .from_node() has the benefit of forwarding more information (including end_lineno and end_colno) that might benefit future error reporting even more.

mparser: remove line=

24bcf37

Even though the current code uses correct lineno/colno, .from_node() has the benefit of forwarding more information (including end_lineno and end_colno) that might benefit future error reporting even more.

Revert "mparser: remove line="

e0315fc

This reverts commit 9a714659ff4abe18a323156fcb7a2534e6c3f556.

Hack to remove line=

0a32da8

WIP

7f64e28

TODO: mparser: switch to providing the sources

8c13758

Fail after mparser: exception trigger source and filename addition

WIP: apparently some NumberNode can be constructed without bytespan

81e23cb

alexandre-janniaux force-pushed the better-error/1 branch from 9a67adf to 81e23cb Compare August 6, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the error reporting in meson #12154

Improve the error reporting in meson #12154

alexandre-janniaux commented Aug 24, 2023 •

edited

Loading

xclaesse commented Aug 24, 2023

alexandre-janniaux commented Aug 24, 2023

tristan957 commented Aug 25, 2023 •

edited

Loading

tristan957 Sep 6, 2023

tristan957 Sep 6, 2023

tristan957 Sep 6, 2023

tristan957 Sep 6, 2023

tristan957 commented Sep 6, 2023

alexandre-janniaux commented Oct 8, 2023

tristan957 commented Oct 9, 2023

alexandre-janniaux commented Oct 10, 2023

tristan957 commented Mar 16, 2024

alexandre-janniaux commented Mar 18, 2024

tristan957 commented Aug 7, 2024

alexandre-janniaux commented Aug 8, 2024

Improve the error reporting in meson #12154

Are you sure you want to change the base?

Improve the error reporting in meson #12154

Conversation

alexandre-janniaux commented Aug 24, 2023 • edited Loading

xclaesse commented Aug 24, 2023

alexandre-janniaux commented Aug 24, 2023

tristan957 commented Aug 25, 2023 • edited Loading

tristan957 Sep 6, 2023

Choose a reason for hiding this comment

tristan957 Sep 6, 2023

Choose a reason for hiding this comment

tristan957 Sep 6, 2023

Choose a reason for hiding this comment

tristan957 Sep 6, 2023

Choose a reason for hiding this comment

tristan957 commented Sep 6, 2023

alexandre-janniaux commented Oct 8, 2023

tristan957 commented Oct 9, 2023

alexandre-janniaux commented Oct 10, 2023

tristan957 commented Mar 16, 2024

alexandre-janniaux commented Mar 18, 2024

tristan957 commented Aug 7, 2024

alexandre-janniaux commented Aug 8, 2024

alexandre-janniaux commented Aug 24, 2023 •

edited

Loading

tristan957 commented Aug 25, 2023 •

edited

Loading