Skip to content

Commit

Permalink
Improved error message when referencing undefined BUILD symbols (pant…
Browse files Browse the repository at this point in the history
…sbuild#19286)

Include the location in the BUILD file (or macro/prelude file) where the
undefined symbol was referenced.

Also preserves the casing of the error message, to not skew non lower
case symbol names in the error message which can be quite
misleading/confusing.
  • Loading branch information
kaos authored Jul 1, 2023
1 parent 3b52f36 commit 062ec62
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 17 deletions.
11 changes: 8 additions & 3 deletions src/python/pants/engine/internals/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import re
import threading
import tokenize
import traceback
from dataclasses import dataclass
from difflib import get_close_matches
from io import StringIO
Expand Down Expand Up @@ -419,15 +420,19 @@ def parse(
code = compile(build_file_content, filepath, "exec", dont_inherit=True)
exec(code, global_symbols)
except NameError as e:
valid_symbols = sorted(s for s in global_symbols.keys() if s != "__builtins__")
original = e.args[0].capitalize()
frame = traceback.extract_tb(e.__traceback__, limit=-1)[0]
msg = ( # Capitalise first letter of NameError message.
e.args[0][0].upper() + e.args[0][1:]
)
location = f":{frame.name}" if frame.name != "<module>" else ""
original = f"{frame.filename}:{frame.lineno}{location}: {msg}"
help_str = softwrap(
f"""
If you expect to see more symbols activated in the below list, refer to
{doc_url('enabling-backends')} for all available backends to activate.
"""
)

valid_symbols = sorted(s for s in global_symbols.keys() if s != "__builtins__")
candidates = get_close_matches(build_file_content, valid_symbols)
if candidates:
if len(candidates) == 1:
Expand Down
93 changes: 79 additions & 14 deletions src/python/pants/engine/internals/parser_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@

from __future__ import annotations

from textwrap import dedent
from typing import Any

import pytest

from pants.build_graph.build_file_aliases import BuildFileAliases
Expand All @@ -20,6 +23,7 @@
from pants.testutil.pytest_util import no_exception
from pants.util.docutil import doc_url
from pants.util.frozendict import FrozenDict
from pants.util.strutil import softwrap


@pytest.fixture
Expand Down Expand Up @@ -72,22 +76,24 @@ def perform_test(extra_targets: list[str], dym: str) -> None:
with pytest.raises(ParseError) as exc:
parser.parse(
"dir/BUILD",
"fake",
"FAKE",
prelude_symbols,
EnvironmentVars({}),
False,
defaults_parser_state,
dependents_rules=None,
dependencies_rules=None,
)
assert str(exc.value) == (
f"Name 'fake' is not defined.\n\n{dym}"
"If you expect to see more symbols activated in the below list,"
f" refer to {doc_url('enabling-backends')} for all available"
" backends to activate.\n\n"
"All registered symbols: ['__defaults__', '__dependencies_rules__', "
f"'__dependents_rules__', 'build_file_dir', 'caof', 'env', {fmt_extra_sym}"
"'obj', 'prelude', 'tgt']"
assert str(exc.value) == softwrap(
f"""
dir/BUILD:1: Name 'FAKE' is not defined.
{dym}If you expect to see more symbols activated in the below list, refer to
{doc_url('enabling-backends')} for all available backends to activate.
All registered symbols: [{fmt_extra_sym}'__defaults__', '__dependencies_rules__',
'__dependents_rules__', 'build_file_dir', 'caof', 'env', 'obj', 'prelude', 'tgt']
"""
)

with no_exception():
Expand All @@ -102,7 +108,7 @@ def perform_test(extra_targets: list[str], dym: str) -> None:
)
parser.parse(
"dir/BUILD",
"fake",
"FAKE",
prelude_symbols,
EnvironmentVars({}),
False,
Expand All @@ -111,17 +117,76 @@ def perform_test(extra_targets: list[str], dym: str) -> None:
dependencies_rules=None,
)

test_targs = ["fake1", "fake2", "fake3", "fake4", "fake5"]
test_targs = ["FAKE1", "FAKE2", "FAKE3", "FAKE4", "FAKE5"]

perform_test([], "")
dym_one = "Did you mean fake1?\n\n"
dym_one = "Did you mean FAKE1?\n\n"
perform_test(test_targs[:1], dym_one)
dym_two = "Did you mean fake2 or fake1?\n\n"
dym_two = "Did you mean FAKE2 or FAKE1?\n\n"
perform_test(test_targs[:2], dym_two)
dym_many = "Did you mean fake5, fake4, or fake3?\n\n"
dym_many = "Did you mean FAKE5, FAKE4, or FAKE3?\n\n"
perform_test(test_targs, dym_many)


@pytest.mark.parametrize("symbol", ["a", "bad", "BAD", "a___b_c", "a231", "áç"])
def test_extract_symbol_from_name_error(symbol: str) -> None:
assert _extract_symbol_from_name_error(NameError(f"name '{symbol}' is not defined")) == symbol


def test_unrecognized_symbol_in_prelude(
defaults_parser_state: BuildFileDefaultsParserState,
) -> None:
build_file_aliases = BuildFileAliases(
objects={"obj": 0},
context_aware_object_factories={"caof": lambda parse_context: lambda _: None},
)
parser = Parser(
build_root="",
registered_target_types=RegisteredTargetTypes({}),
union_membership=UnionMembership({}),
object_aliases=build_file_aliases,
ignore_unrecognized_symbols=False,
)
prelude: dict[str, Any] = {}
exec(
compile(
dedent(
"""\
# This macro references some undefined symbol...
def macro():
return NonExisting
"""
),
"preludes/bad.py",
"exec",
dont_inherit=True,
),
{},
prelude,
)
prelude_symbols = BuildFilePreludeSymbols.create(prelude, ())

with pytest.raises(ParseError) as exc:
parser.parse(
filepath="dir/BUILD",
build_file_content="macro()",
extra_symbols=prelude_symbols,
env_vars=EnvironmentVars({}),
is_bootstrap=False,
defaults=defaults_parser_state,
dependents_rules=None,
dependencies_rules=None,
)
assert str(exc.value) == softwrap(
f"""
preludes/bad.py:3:macro: Name 'NonExisting' is not defined.
Did you mean macro?
If you expect to see more symbols activated in the below list, refer to
{doc_url('enabling-backends')} for all available backends to activate.
All registered symbols: ['__defaults__', '__dependencies_rules__', '__dependents_rules__',
'build_file_dir', 'caof', 'env', 'macro', 'obj']
"""
)

0 comments on commit 062ec62

Please sign in to comment.