Ruff makes triple sure the user is aware an error code has been remapped when it has explictly been ignored

[onerandomusername]

Summary

When ignoring a remapped error code, ruff is very insistent in error logs that the error code has been remapped.

Steps to reproduce

pyproject.toml:

[tool.ruff]
select = ["RUF"]
ignore = ["RUF004"]

command: ruff .
There does not need to be any python files in the current directory.

Screenshots

[charliermarsh]

I think we used to show this once, but a refactor made it more difficult to enforce that it appears "exactly once".

[onerandomusername]

IMO it would be better at the end of cli output rather than the beginning

[MithicSpirit]

It seems that this warning is coming from https://github.com/charliermarsh/ruff/blob/bb3bb24b59d8f0da5c42972b177cd29db9390d6f/crates/ruff/src/settings/mod.rs#L362-L369
Is the issue that the function (RuleTable::from) is being called multiple times or is it just that there are multiple instances of the value in redirects? If it's the latter it shouldn't be hard to deduplicate, although the fact that it seems to be iterating over a hashmap makes me doubtful that this is the case.

[charliermarsh]

I think it's that we may call RuleTable::from multiple times. We actually do have a mechanism to show a warning "exactly once", but it doesn't work with dynamic warnings (i.e., warnings like this that depend on data), since it basically inlines a one-time static boolean into the code via a macro.

[MithicSpirit]

Yeah I looked into that macro but realized that it wouldn't fix it. I think that it could be adapted for dynamic warnings like these by using a static hashmap with the appropriate locks, but I'm not sure if the overhead from that is worth it.

[charliermarsh]

I think it's probably worth it, since this isn't a high-performance codepath. Are you interested in giving that a try?

[charliermarsh]

I started on a kind of ridiculous change to use macros in rule_redirects.rs, like:

/// Returns the redirect target for the given code.
pub(crate) fn get_redirect_target(code: &str) -> Option<&'static str> {
    redirect(code).map(|(_, target)| target)
}

/// Returns the code and the redirect target if the given code is a redirect.
/// (The same code is returned to obtain it with a static lifetime).
pub(crate) fn get_redirect(code: &str) -> Option<(&'static str, &'static str)> {
    redirect(code)
}

fn redirect(code: &str) -> Option<(&'static str, &'static str)> {
    macro_rules! redirect {
        ($src:expr, $dst:expr) => {{
            if code == $src {
                crate::warn_user_once!("`{}` has been remapped to `{}`.", $src, $dst);
                return Some(($src, $dst));
            }
        }};
    }

    redirect!("SIM111", "SIM110");

    return None;
}

That actually does work, but the issue is that the redirect itself is done before we set up the logger (e.g., if the codes are provided as command-line arguments), so the warning doesn't show up in all cases because it's "too soon".

[MithicSpirit]

I think it's probably worth it, since this isn't a high-performance codepath. Are you interested in giving that a try?

I can give it a shot, but I should say I'm not too familiar with atomics (might be a good opportunity to learn, though).

Since this function is not async, things like Mutex and RwLock are out. I'm thinking that the best option would be to use a static FxHashSet to keep track of which redirects have already been printed and a static AtomicBool to act as a lock on the hashset. From there I believe that it would be necessary to use a spinlock (again, because this function is not async); while this isn't great, the lock will be held only briefly, so there shouldn't be much time wasted spinlocking. What do you think about this solution?

[onerandomusername]

That actually does work, but the issue is that the redirect itself is done before we set up the logger (e.g., if the codes are provided as command-line arguments), so the warning doesn't show up in all cases because it's "too soon".

If a refactor is necessary than I also think it might be better to show warnings like this near the end of the output.

[MithicSpirit]

We could make the redirects FxHashMap static (and do the whole spinlocking thing while writing to it), then wait until all of the linting is done to print it out. This would require more refactoring than my solution, but probably less than @charliermarsh's current solution.

[charliermarsh]

I can help with this one.