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

Incorrect NoQA placement for continuation with multi-line string #7530

Closed
dhruvmanila opened this issue Sep 20, 2023 · 0 comments · Fixed by #7531
Closed

Incorrect NoQA placement for continuation with multi-line string #7530

dhruvmanila opened this issue Sep 20, 2023 · 0 comments · Fixed by #7531
Labels
bug Something isn't working suppression Related to supression of violations e.g. noqa

Comments

@dhruvmanila
Copy link
Member

dhruvmanila commented Sep 20, 2023

Take the following code snippet:

assert foo, \
    """triple-quoted
    string"""

Running the following command repeatedly:

$ ruff check --no-cache --isolated --select=F821 fstring.py --add-noqa
Added 1 noqa directive.

$ ruff check --no-cache --isolated --select=F821 fstring.py --add-noqa
Added 1 noqa directive.

...

It keeps on adding the noqa directive inside the first line of the string:

assert foo, \
    """triple-quoted  # noqa: F821  # noqa: F821
    string"""

I think the problem is in the way the range is being pushed onto the NoQA mapping in the following code:

pub(crate) fn push_mapping(&mut self, range: TextRange) {
if let Some(last_range) = self.ranges.last_mut() {
// Strictly sorted insertion
if last_range.end() <= range.start() {
// OK
}
// Try merging with the last inserted range
else if let Some(intersected) = last_range.intersect(range) {
*last_range = intersected;
return;
} else {
panic!("Ranges must be inserted in sorted order")
}
}

  1. The <= doesn't make it strict
  2. We should use the union range instead of intersected range

So,

    pub(crate) fn push_mapping(&mut self, range: TextRange) {
        if let Some(last_range) = self.ranges.last_mut() {
            // Strictly sorted insertion
            if last_range.end() < range.start() {
                // OK
            } else if range.end() < last_range.start() {
                panic!("Ranges must be inserted in sorted order")
            } else {
                *last_range = last_range.cover(range);
                return;
            }
        }

        self.ranges.push(range);
    }
@dhruvmanila dhruvmanila added bug Something isn't working suppression Related to supression of violations e.g. noqa labels Sep 20, 2023
dhruvmanila added a commit that referenced this issue Sep 20, 2023
## Summary

This PR fixes the way NoQA range is inserted to the `NoqaMapping`.

Previously, the way the mapping insertion logic worked was as follows:
1. If the range which is to be inserted _touched_ the previous range, meaning
that the end of the previous range was the same as the start of the new
range, then the new range was added in addition to the previous range.
2. Else, if the new range intersected the previous range, then the previous
   range was replaced with the new _intersection_ of the two ranges.

The problem with this logic is that it does not work for the following case:
```python
assert foo, \
    """multi-line
    string"""
```

Now, the comments cannot be added to the same line which ends with a continuation
character. So, the `NoQA` directive has to be added to the next line. But, the
next line is also a triple-quoted string, so the `NoQA` directive for that line
needs to be added to the next line. This creates a **union** pattern instead of an
**intersection** pattern.

But, only union doesn't suffice because (1) means that for the edge case where
the range touch only at the end, the union won't take place.

### Solution

1. Replace '<=' with '<' to have a _strict_ insertion case
2. Use union instead of intersection

## Test Plan

Add a new test case. Run the test suite to ensure that nothing is broken.

### Integration

1. Make a `test.py` file with the following contents:

    ```python
    assert foo, \
        """multi-line
        string"""
    ```

2. Run the following command:

    ```console
	$ cargo run --bin ruff -- check --isolated --no-cache --select=F821 test.py
	/Users/dhruv/playground/ruff/fstring.py:1:8: F821 Undefined name `foo`
    Found 1 error.
    ```

3. Use `--add-noqa`:

    ```console
	$ cargo run --bin ruff -- check --isolated --no-cache --select=F821 --add-noqa test.py
    Added 1 noqa directive.
    ```

4. Check that the NoQA directive was added in the correct position:

    ```python
    assert foo, \
        """multi-line
        string"""  # noqa: F821
    ```

5. Run the `check` command to ensure that the NoQA directive is respected:

    ```console
	$ cargo run --bin ruff -- check --isolated --no-cache --select=F821 test.py
    ```

fixes: #7530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working suppression Related to supression of violations e.g. noqa
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant