Failure to parse `ruff.toml` results in default configuration

[lpulley]

When ruff.toml changes (e.g. git checkout) and the new contents are unparseable by the running Ruff server (e.g. addition of builtins-strict-checking with an already-running pre-0.9.6 Ruff server), the Ruff extension apparently just falls back to the default configuration and will still apply formatting changes, fixes, etc. disregarding ruff.toml. For us, this resulted in files being auto-formatted on save in ways that disagree with ruff.toml.

It seems to me that if ruff.toml fails to parse with the running Ruff server, the extension should at least try restarting it (in case the Ruff version launched by the ruff.path executable has been changed to cooperate with the changes to ruff.toml), and at the very least, it shouldn't go ahead with the default configuration. In other words, if there is a failure to parse ruff.toml, I never want the Ruff extension to ignore it and just try using the default configuration; I would want it to stop.

[dhruvmanila]

The server does register a watcher for configuration files in the current project but that's mainly to refresh the diagnostics whenever the config file changes (https://docs.astral.sh/ruff/editors/features/#dynamic-configuration).

I think this is a general problem and #701 is in the similar category. We do notify the user about the failure in the recent versions, do you think that helps somewhat in mitigating the issue you're facing?

What to do in case the server failed to parse one or more config files present in the project? This failure could occur for various reasons like invalid syntax, invalid schema, etc.

The current behavior is that we ignore the config files that raised any kind of error. Then, when a document that would use that config is opened, it falls back to the default configuration as the indexer doesn't contain the (failed due to an error) config.

[lpulley]

We do notify the user about the failure in the recent versions, do you think that helps somewhat in mitigating the issue you're facing?

I noticed it, and I assumed it meant "the Ruff extension won't work" or "the Ruff extension failed to start".

The current behavior is that we ignore the config files that raised any kind of error. Then, when a document that would use that config is opened, it falls back to the default configuration as the indexer doesn't contain the (failed due to an error) config.

I think this ("ignore the config files that raised any kind of error" / "falls back to the default configuration") is not what users would expect. Personally, I'd suggest making it fail if it fails to read a config file, with a VS Code setting to return to the current behavior (falling back to the default configuration) if for some reason someone prefers that.

[lpulley]

Put differently: Ruff itself blows up when faced with a bad config file, and I expected the extension to do that, too.

[lpulley]

It would also be convenient for us if the Ruff extension would first attempt to restart the Ruff server, since that would have resulted in everything working in our case, but I understand that might not always be a thing worth trying. 🤷🏻

[dhruvmanila]

I noticed it, and I assumed it meant "the Ruff extension won't work" or "the Ruff extension failed to start".

We could use an explicit message stating that the server will fallback to default settings and the user might see unexpected behavior and recommend them to fix the settings.

I think this ("ignore the config files that raised any kind of error" / "falls back to the default configuration") is not what users would expect. Personally, I'd suggest making it fail if it fails to read a config file, with a VS Code setting to return to the current behavior (falling back to the default configuration) if for some reason someone prefers that.

Thank you for the feedback. I think I'd prefer to update the message to be explicit about the server behavior and wait for more user feedback regarding this.

Put differently: Ruff itself blows up when faced with a bad config file, and I expected the extension to do that, too.

I don't think we can do that in the extension as otherwise it might lead to even worse behavior. Let me provide you some context. The extension works using the ruff server (language server protocol implementation) as the backend. This means that the editor asks sends requests to provide diagnostics, formatting, etc. to the server which then responds accordingly. Now, the config is read on the server side which if we were to fail (in this context, it would mean to stop the server) then VS Code would probably try to restart the server a maximum of 5 times and then stop retrying. Or, worse the extension might need to be restarted.

It would also be convenient for us if the Ruff extension would first attempt to restart the Ruff server, since that would have resulted in everything working in our case, but I understand that might not always be a thing worth trying. 🤷🏻

Currently, we do refresh the diagnostics whenever any of the valid config files have changed but not for invalid ones.

[MichaReiser]

It could make sense for us to store whether loading the configuration was successful or not and disable code actions (format on save, organize imports on save, etc) when that's true ( and show a message that the request was ignored because the configuration is broken.

A user can still format the document by manually requesting a Format document but that doesn't seem as problematic because the user explicitly requests to format the document.

I don't know how hard it is to implement this feature.

[lpulley]

Micha, I think that's a great idea. That way, the extension wouldn't be totally broken, but we'd prevent surprise reformats like we experienced.

[dhruvmanila]

It could make sense for us to store whether loading the configuration was successful or not and disable code actions (format on save, organize imports on save, etc) when that's true ( and show a message that the request was ignored because the configuration is broken.

A user can still format the document by manually requesting a Format document but that doesn't seem as problematic because the user explicitly requests to format the document.

I don't know how hard it is to implement this feature.

Yeah, I think that would be better. It's kind of related to #701 where we want something similar.