Add strict flag

[fortmarek]

Hi!

I'd find it quite useful to be able to promote warnings to be errors when running swift-format lint.

My use-case is that I'd like to validate on CI that no formatting is needed but we want to leave it up to the developer to fix it - this is very similar to what eg. swiftlint offers (and swiftformat offers a similar functionality as well, for that matter).
The solution is quite simple and more-or-less copies if frontend.errorsWereEmitted { throw ExitCode.failure }.

This PR is only a draft to validate whether such feature would be welcome, so there are no tests (I have validated this only by running it locally).

Sorry if this is already possible and I have missed it 🙇 (I have tried to see if there's any mention of this in the repo and in the previous PRs, but have not found any)

[allevato]

Thanks for the contribution!

I somewhat think that this feature would be served better by having it actually upgrade the severity of the diagnostics themselves to .error instead of .warning, and then the extra check wouldn't be necessary. But that would also require a significant refactoring of the way we currently create those diagnostics... and I don't think we should do that yet, because there are other changes that may be worth doing at the same time as well (like letting the severity of warnings be controlled on a case-by-case basis in the configuration file).

So what you have here seems like a reasonable approach in the meantime.

Can you rebase your change on top of the main branch? (I know it doesn't build against a current development snapshot; unfortunately I haven't had cycles to update it based on recent syntax changes upstream, but I can try to do that soon in order to merge this in.)

[fortmarek]

Hey @allevato! I have rebased on top of main - which has made the diff between 5.3 branch and this PR quite larger (plus it generated a bunch of merge conflicts with the current branch). Should I keep pointing to swift-5.3-branch or change it to main?

I somewhat think that this feature would be served better by having it actually upgrade the severity of the diagnostics themselves to .error instead of .warning

I agree, let me know if you want help with the subsequent refactor. Promoting warnings to errors themselves would also enable me to write tests - I am not sure if it is possible at the moment to do so as the changes are limited to Lint command.

[allevato]

Hey @allevato! I have rebased on top of main - which has made the diff between 5.3 branch and this PR quite larger (plus it generated a bunch of merge conflicts with the current branch). Should I keep pointing to swift-5.3-branch or change it to main?

Yes, please point the PR at main.

I agree, let me know if you want help with the subsequent refactor. Promoting warnings to errors themselves would also enable me to write tests - I am not sure if it is possible at the moment to do so as the changes are limited to Lint command.

I'm always happy to discuss design proposals and review changes! Since it hasn't felt terribly urgent, I haven't given it a great deal of thought yet so I don't have a strong lean about how to best handle the change. The one immediate issue that comes to mind is that our diagnostic values are created completely context-less, and they have the severity baked in. So if we wanted to move to a model where the Configuration could determine the severity of diagnostics at runtime instead, we'd probably need to create a new formatter-specific diagnostic manager abstraction that owns an instance of the Configuration and accepts a slightly re-modeled notion of what a diagnostic value is, and then transforms those into the actual SwiftSyntax diagnostics.

We also can't tie the "category" of a diagnostic 1:1 to the "rule" concept because we emit diagnostics from other places as well, such as the whitespace linter, and the frontend itself. Heck, some rules emit multiple kinds of diagnostics, and someone might want to control their severity independently (we'd have to decide if that makes sense and if we want to allow that level of granularity).

Unfortunately these changes would ripple through the entire codebase—we'd probably need to re-model the Configuration quite a bit (both in its in-memory and on-disk representations), and the unit tests would be pretty deeply affected as well, where we use the diagnostic values directly for assertions. 😭

I'll try to think on it some more and see where we can get with smaller incremental changes.

[fortmarek]

Yes, please point the PR at main.

Done!

we'd probably need to create a new formatter-specific diagnostic manager abstraction that owns an instance of the Configuration and accepts a slightly re-modeled notion of what a diagnostic value is, and then transforms those into the actual SwiftSyntax diagnostics.

This approach seems sensible to me - but as you said, it can create a lot of changes. But for the long run, it is probably beneficial to have a stronger control over the diagnostics. Once we'd have our own diagnostics engine layer, promoting warnings to errors (or some other modification of the severity) could be done very easily.

[allevato]

This looks good now, thank you!