astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.79k stars 1.09k forks source link

False negative with useless-expression on strings #11292

Open NeilGirdhar opened 6 months ago

NeilGirdhar commented 6 months ago

For some reason useless-expression is not triggered by loose strings. Should these be caught? If comments are intended, then Ruff could suggest converting them comments?

class X:
    """Some docstring."""

def f() -> None:
    """Some docstring."""
    [123]  # noqa: B018
    "Some loose string."  # Should be B018.

x = 1
"""Some editors consider this a variable docstring, but it doesn't affect any __doc__ and
ordinary comments are ubiquitous."""
charliermarsh commented 6 months ago

The intention here is to avoid flagging docstrings, but I think we need to make it more targeted.

NeilGirdhar commented 6 months ago

Yeah, that's what I figured. I tried to illustrate some reasonable uses of strings. I'm not sure how you feel about the "variable docstring", which some editors recognize. As far as I know, they're rarely used and not really a Python concept. They look pretty weird in code:

length: int  # The length of the object.
width: int  # The width of the object.

versus

length: int
"The length of the object."
width: int
"The width of the object."
charliermarsh commented 6 months ago

Yeah, I think we do need to ignore attribute docstrings. They're not standardized, but they're popular enough that I think it'd be a net-negative to flag them.

dhruvmanila commented 6 months ago

I think this can be fixed because our docstring detection is quite accurate now.

dhruvmanila commented 5 months ago

It seems that updating B018 seems to raise a lot of diagnostics in the ecosystem checks. What about implementing the PLW0105 rule from Pylint instead of updating B018? This will also match flake8-bugbear behavior.

NeilGirdhar commented 5 months ago

Please don't use the same documentation as PLW0105: strings are not reasonable documentation in Python, and I don't think the linter should suggest that.

estyrke commented 4 months ago

If B018 is to be modified, consider the special case of useless string expression after a string assignment. This is rather nasty and hard to detect for a human (the correct code should have parentheses to make this a multi line implicit string concatenation):

    def __str__(self) -> str:
        string = f"AnalysisConfig(ID: {self.uuid},"
        f"BackendURL: {self.backend_url},"
        f"AnnotationTypes: {self.annotation_types})"
        return string

For that matter, any case with an f-string is probably always an error, since it would be pointless to use those for comments.

NeilGirdhar commented 4 months ago

Maybe Ruff should flag "attribute docstrings" using a separate rule to convert them to annotated annotations?

x: int
"blah"

becomes

x: Annotated[int, "blah"]

This is more consistent with modern Python.