Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid raising bare Exception #1168

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions libcst/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# LICENSE file in the root directory of this source tree.

from libcst._batched_visitor import BatchableCSTVisitor, visit_batched
from libcst._excep import CSTLogicError
from libcst._exceptions import MetadataException, ParserSyntaxError
from libcst._flatten_sentinel import FlattenSentinel
from libcst._maybe_sentinel import MaybeSentinel
Expand Down Expand Up @@ -238,6 +239,7 @@
"CSTNodeT",
"CSTTransformer",
"CSTValidationError",
"CSTLogicError",
"CSTVisitor",
"CSTVisitorT",
"FlattenSentinel",
Expand Down
8 changes: 8 additions & 0 deletions libcst/_excep.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Copyright (c) Meta Platforms, Inc. and affiliates.
#
# This source code is licensed under the MIT license found in the
# LICENSE file in the root directory of this source tree.


class CSTLogicError(Exception):
pass
Comment on lines +1 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in _exceptions.py, and please write a docstring

Copy link
Contributor Author

@zaicruvoir1rominet zaicruvoir1rominet Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review !

I'm going to need help on this one though:

  • It's in a separate _excep.py file, since _exceptions.py already imports internals, which in turn may raise a CSTLogicError.
    Do you see any other ways than my (I admit lazy) solution in order to avoid circular import errors ?
  • For the docstring, I couldn't think of anything meaningful, other than "Well... it's a CST Logic error", which doesn't add more info or context than the original name. I'd be glad to add a real doc if you have something better in mind :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zsol
Any updates ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_exceptions.py already imports internals, which in turn may raise a CSTLogicError.

Let's move get_expected_str out of _exceptions.py. That should help.

For the docstring, I couldn't think of anything meaningful

How about:

General purpose internal error within LibCST itself.

9 changes: 5 additions & 4 deletions libcst/_nodes/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from dataclasses import dataclass, field, fields, replace
from typing import Any, cast, ClassVar, Dict, List, Mapping, Sequence, TypeVar, Union

from libcst._excep import CSTLogicError
from libcst._flatten_sentinel import FlattenSentinel
from libcst._nodes.internal import CodegenState
from libcst._removal_sentinel import RemovalSentinel
Expand Down Expand Up @@ -237,7 +238,7 @@

# validate return type of the user-defined `visitor.on_leave` method
if not isinstance(leave_result, (CSTNode, RemovalSentinel, FlattenSentinel)):
raise Exception(
raise CSTValidationError(

Check warning on line 241 in libcst/_nodes/base.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/base.py#L241

Added line #L241 was not covered by tests
"Expected a node of type CSTNode or a RemovalSentinel, "
+ f"but got a return value of {type(leave_result).__name__}"
)
Expand Down Expand Up @@ -383,7 +384,7 @@
new_tree = self.visit(_ChildReplacementTransformer(old_node, new_node))
if isinstance(new_tree, (FlattenSentinel, RemovalSentinel)):
# The above transform never returns *Sentinel, so this isn't possible
raise Exception("Logic error, cannot get a *Sentinel here!")
raise CSTLogicError("Logic error, cannot get a *Sentinel here!")

Check warning on line 387 in libcst/_nodes/base.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/base.py#L387

Added line #L387 was not covered by tests
return new_tree

def deep_remove(
Expand All @@ -400,7 +401,7 @@

if isinstance(new_tree, FlattenSentinel):
# The above transform never returns FlattenSentinel, so this isn't possible
raise Exception("Logic error, cannot get a FlattenSentinel here!")
raise CSTLogicError("Logic error, cannot get a FlattenSentinel here!")

Check warning on line 404 in libcst/_nodes/base.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/base.py#L404

Added line #L404 was not covered by tests

return new_tree

Expand All @@ -422,7 +423,7 @@
new_tree = self.visit(_ChildWithChangesTransformer(old_node, changes))
if isinstance(new_tree, (FlattenSentinel, RemovalSentinel)):
# This is impossible with the above transform.
raise Exception("Logic error, cannot get a *Sentinel here!")
raise CSTLogicError("Logic error, cannot get a *Sentinel here!")

Check warning on line 426 in libcst/_nodes/base.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/base.py#L426

Added line #L426 was not covered by tests
return new_tree

def __eq__(self: _CSTNodeSelfT, other: object) -> bool:
Expand Down
7 changes: 4 additions & 3 deletions libcst/_nodes/expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from typing import Callable, Generator, Literal, Optional, Sequence, Union

from libcst._add_slots import add_slots
from libcst._excep import CSTLogicError
from libcst._maybe_sentinel import MaybeSentinel
from libcst._nodes.base import CSTCodegenError, CSTNode, CSTValidationError
from libcst._nodes.internal import (
Expand Down Expand Up @@ -666,7 +667,7 @@
if len(quote) not in {1, 3}:
# We shouldn't get here due to construction validation logic,
# but handle the case anyway.
raise Exception(f"Invalid string {self.value}")
raise CSTLogicError(f"Invalid string {self.value}")

Check warning on line 670 in libcst/_nodes/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/expression.py#L670

Added line #L670 was not covered by tests

# pyre-ignore We know via the above validation that we will only
# ever return one of the four string literals.
Expand Down Expand Up @@ -1010,7 +1011,7 @@
elif isinstance(right, FormattedString):
rightbytes = "b" in right.prefix
else:
raise Exception("Logic error!")
raise CSTLogicError("Logic error!")

Check warning on line 1014 in libcst/_nodes/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/expression.py#L1014

Added line #L1014 was not covered by tests
if leftbytes != rightbytes:
raise CSTValidationError("Cannot concatenate string and bytes.")

Expand Down Expand Up @@ -1688,7 +1689,7 @@
if default_indicator == "->":
state.add_token(" ")
else:
raise Exception("Logic error!")
raise CSTLogicError("Logic error!")

Check warning on line 1692 in libcst/_nodes/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/expression.py#L1692

Added line #L1692 was not covered by tests

# Now, output the indicator and the rest of the annotation
state.add_token(default_indicator)
Expand Down
13 changes: 6 additions & 7 deletions libcst/_nodes/statement.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from typing import Literal, Optional, Pattern, Sequence, Union

from libcst._add_slots import add_slots
from libcst._excep import CSTLogicError
from libcst._maybe_sentinel import MaybeSentinel
from libcst._nodes.base import CSTNode, CSTValidationError
from libcst._nodes.expression import (
Expand Down Expand Up @@ -1161,12 +1162,10 @@ def _validate(self) -> None:
)
try:
self.evaluated_name
except Exception as e:
if str(e) == "Logic error!":
raise CSTValidationError(
"The imported name must be a valid qualified name."
)
raise e
except CSTLogicError as e:
raise CSTValidationError(
"The imported name must be a valid qualified name."
) from e

def _visit_and_replace_children(self, visitor: CSTVisitorT) -> "ImportAlias":
return ImportAlias(
Expand Down Expand Up @@ -1195,7 +1194,7 @@ def _name(self, node: CSTNode) -> str:
elif isinstance(node, Attribute):
return f"{self._name(node.value)}.{node.attr.value}"
else:
raise Exception("Logic error!")
raise CSTLogicError("Logic error!")

@property
def evaluated_name(self) -> str:
Expand Down
4 changes: 3 additions & 1 deletion libcst/_nodes/tests/test_funcdef.py
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,9 @@
code, config=cst.PartialParserConfig(python_version="3.8")
)
if not isinstance(statement, cst.BaseCompoundStatement):
raise Exception("This function is expecting to parse compound statements only!")
raise ValueError(

Check warning on line 1055 in libcst/_nodes/tests/test_funcdef.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/tests/test_funcdef.py#L1055

Added line #L1055 was not covered by tests
"This function is expecting to parse compound statements only!"
)
return statement


Expand Down
4 changes: 3 additions & 1 deletion libcst/_nodes/tests/test_namedexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
code, config=cst.PartialParserConfig(python_version="3.8")
)
if not isinstance(statement, cst.BaseCompoundStatement):
raise Exception("This function is expecting to parse compound statements only!")
raise ValueError(

Check warning on line 25 in libcst/_nodes/tests/test_namedexpr.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/tests/test_namedexpr.py#L25

Added line #L25 was not covered by tests
"This function is expecting to parse compound statements only!"
)
return statement


Expand Down
2 changes: 1 addition & 1 deletion libcst/_nodes/tests/test_removal_behavior.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
self, before: str, after: str, visitor: Type[CSTTransformer]
) -> None:
if before.endswith("\n") or after.endswith("\n"):
raise Exception("Test cases should not be newline-terminated!")
raise ValueError("Test cases should not be newline-terminated!")

Check warning on line 98 in libcst/_nodes/tests/test_removal_behavior.py

View check run for this annotation

Codecov / codecov/patch

libcst/_nodes/tests/test_removal_behavior.py#L98

Added line #L98 was not covered by tests

# Test doesn't have newline termination case
before_module = parse_module(before)
Expand Down
2 changes: 1 addition & 1 deletion libcst/_parser/base_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
def parse(self) -> _NodeT:
# Ensure that we don't re-use parsers.
if self.__was_parse_called:
raise Exception("Each parser object may only be used to parse once.")
raise ValueError("Each parser object may only be used to parse once.")

Check warning on line 106 in libcst/_parser/base_parser.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/base_parser.py#L106

Added line #L106 was not covered by tests
self.__was_parse_called = True

for token in self.tokens:
Expand Down
56 changes: 46 additions & 10 deletions libcst/_parser/conversions/expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
Intnumber as INTNUMBER_RE,
)

from libcst._exceptions import PartialParserSyntaxError
from libcst._excep import CSTLogicError
from libcst._exceptions import ParserSyntaxError, PartialParserSyntaxError
from libcst._maybe_sentinel import MaybeSentinel
from libcst._nodes.expression import (
Arg,
Expand Down Expand Up @@ -327,7 +328,12 @@
# Convert all of the operations that have no precedence in a loop
for op, rightexpr in grouper(rightexprs, 2):
if op.string not in BOOLOP_TOKEN_LUT:
raise Exception(f"Unexpected token '{op.string}'!")
raise ParserSyntaxError(

Check warning on line 331 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L331

Added line #L331 was not covered by tests
f"Unexpected token '{op.string}'!",
lines=config.lines,
raw_line=0,
raw_column=0,
)
leftexpr = BooleanOperation(
left=leftexpr,
# pyre-ignore Pyre thinks that the type of the LUT is CSTNode.
Expand Down Expand Up @@ -420,7 +426,12 @@
)
else:
# this should be unreachable
raise Exception(f"Unexpected token '{op.string}'!")
raise ParserSyntaxError(

Check warning on line 429 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L429

Added line #L429 was not covered by tests
f"Unexpected token '{op.string}'!",
lines=config.lines,
raw_line=0,
raw_column=0,
)
else:
# A two-token comparison
leftcomp, rightcomp = children
Expand Down Expand Up @@ -451,7 +462,12 @@
)
else:
# this should be unreachable
raise Exception(f"Unexpected token '{leftcomp.string} {rightcomp.string}'!")
raise ParserSyntaxError(

Check warning on line 465 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L465

Added line #L465 was not covered by tests
f"Unexpected token '{leftcomp.string} {rightcomp.string}'!",
lines=config.lines,
raw_line=0,
raw_column=0,
)


@with_production("star_expr", "'*' expr")
Expand Down Expand Up @@ -493,7 +509,12 @@
# Convert all of the operations that have no precedence in a loop
for op, rightexpr in grouper(rightexprs, 2):
if op.string not in BINOP_TOKEN_LUT:
raise Exception(f"Unexpected token '{op.string}'!")
raise ParserSyntaxError(

Check warning on line 512 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L512

Added line #L512 was not covered by tests
f"Unexpected token '{op.string}'!",
lines=config.lines,
raw_line=0,
raw_column=0,
)
leftexpr = BinaryOperation(
left=leftexpr,
# pyre-ignore Pyre thinks that the type of the LUT is CSTNode.
Expand Down Expand Up @@ -540,7 +561,12 @@
)
)
else:
raise Exception(f"Unexpected token '{op.string}'!")
raise ParserSyntaxError(

Check warning on line 564 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L564

Added line #L564 was not covered by tests
f"Unexpected token '{op.string}'!",
lines=config.lines,
raw_line=0,
raw_column=0,
)

return WithLeadingWhitespace(
UnaryOperation(operator=opnode, expression=factor.value), op.whitespace_before
Expand Down Expand Up @@ -651,7 +677,7 @@
)
else:
# This is an invalid trailer, so lets give up
raise Exception("Logic error!")
raise CSTLogicError()

Check warning on line 680 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L680

Added line #L680 was not covered by tests
return WithLeadingWhitespace(atom, whitespace_before)


Expand Down Expand Up @@ -870,9 +896,19 @@
Imaginary(child.string), child.whitespace_before
)
else:
raise Exception(f"Unparseable number {child.string}")
raise ParserSyntaxError(

Check warning on line 899 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L899

Added line #L899 was not covered by tests
f"Unparseable number {child.string}",
lines=config.lines,
raw_line=0,
raw_column=0,
)
else:
raise Exception(f"Logic error, unexpected token {child.type.name}")
raise ParserSyntaxError(

Check warning on line 906 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L906

Added line #L906 was not covered by tests
f"Logic error, unexpected token {child.type.name}",
lines=config.lines,
raw_line=0,
raw_column=0,
)


@with_production("atom_squarebrackets", "'[' [testlist_comp_list] ']'")
Expand Down Expand Up @@ -1447,7 +1483,7 @@
if equal.string == ":=":
val = convert_namedexpr_test(config, children)
if not isinstance(val, WithLeadingWhitespace):
raise Exception(
raise TypeError(

Check warning on line 1486 in libcst/_parser/conversions/expression.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/expression.py#L1486

Added line #L1486 was not covered by tests
f"convert_namedexpr_test returned {val!r}, not WithLeadingWhitespace"
)
return Arg(value=val.value)
Expand Down
13 changes: 7 additions & 6 deletions libcst/_parser/conversions/params.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from typing import Any, List, Optional, Sequence, Union

from libcst._excep import CSTLogicError
from libcst._exceptions import PartialParserSyntaxError
from libcst._maybe_sentinel import MaybeSentinel
from libcst._nodes.expression import (
Expand Down Expand Up @@ -121,7 +122,7 @@
# Example code:
# def fn(*abc, *): ...
# This should be unreachable, the grammar already disallows it.
raise Exception(
raise ValueError(

Check warning on line 125 in libcst/_parser/conversions/params.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/params.py#L125

Added line #L125 was not covered by tests
"Cannot have multiple star ('*') markers in a single argument "
+ "list."
)
Expand All @@ -136,7 +137,7 @@
# Example code:
# def fn(foo, /, *, /, bar): ...
# This should be unreachable, the grammar already disallows it.
raise Exception(
raise ValueError(

Check warning on line 140 in libcst/_parser/conversions/params.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/params.py#L140

Added line #L140 was not covered by tests
"Cannot have multiple slash ('/') markers in a single argument "
+ "list."
)
Expand Down Expand Up @@ -168,7 +169,7 @@
# Example code:
# def fn(**kwargs, trailing=None)
# This should be unreachable, the grammar already disallows it.
raise Exception("Cannot have any arguments after a kwargs expansion.")
raise ValueError("Cannot have any arguments after a kwargs expansion.")

Check warning on line 172 in libcst/_parser/conversions/params.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/params.py#L172

Added line #L172 was not covered by tests
elif (
isinstance(param.star, str) and param.star == "*" and param.default is None
):
Expand All @@ -181,7 +182,7 @@
# Example code:
# def fn(*first, *second): ...
# This should be unreachable, the grammar already disallows it.
raise Exception(
raise ValueError(

Check warning on line 185 in libcst/_parser/conversions/params.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/params.py#L185

Added line #L185 was not covered by tests
"Expected a keyword argument but found a starred positional "
+ "argument expansion."
)
Expand All @@ -197,13 +198,13 @@
# Example code:
# def fn(**first, **second)
# This should be unreachable, the grammar already disallows it.
raise Exception(
raise ValueError(

Check warning on line 201 in libcst/_parser/conversions/params.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/params.py#L201

Added line #L201 was not covered by tests
"Multiple starred keyword argument expansions are not allowed in a "
+ "single argument list"
)
else:
# The state machine should never end up here.
raise Exception("Logic error!")
raise CSTLogicError("Logic error!")

Check warning on line 207 in libcst/_parser/conversions/params.py

View check run for this annotation

Codecov / codecov/patch

libcst/_parser/conversions/params.py#L207

Added line #L207 was not covered by tests

return current_param

Expand Down
Loading
Loading