Proposal: Default rule list, move default rules to bugprone

[ZedThree]

Closes #306

This is a proposal for a (hopefully) sensible set of default rules where I've moved them into the bugprone category. This makes the default rules easy to remember -- ["E", "B", "OB"].

Although I've fixed some tests, I've not moved any files around, and some test outputs now include the warning for redirected rules.

Testing this out on real world projects seems to work fine, ignores and selects are all redirected correctly.

One weird side-effect is that the M category is now all preview rules, so selecting it will warn it does nothing without enabling preview mode.

Perhaps the main downside is that we check fewer rules by default, but it's trivial to reenable everything with --select=ALL

[LiamPattinson]

Looking through the rules, there are a couple that I think should perhaps be recategorised:

• Should assumed-size also be in bugprone?
• deprecated-assumed-size-character feels like a style rule more than bugprone. I believe assumed-size-character-intent will catch this syntax, though oddly we don't test for it.

I'm also looking at all of the rules for handling real types/kinds, and thinking perhaps it was a bad idea to split them between typing and precision. I'm considering moving all of them to a category floating-point/FL and deprecating the precision category entirely. While we're at it, I'd also like to move star-kind to the style category. Do you think that's something I should do as a separate PR?

---

Although I've fixed some tests, I've not moved any files around, and some test outputs now include the warning for redirected rules.

Could we move things around before the merge? It's probably best for us to figure out exactly where things should be beforehand, so I think that should be the last thing we do in this PR.

---

One weird side-effect is that the M category is now all preview rules, so selecting it will warn it does nothing without enabling preview mode.

I think that's okay, as I'd like to move a bunch of rules out of preview for the next release anyway.

[ZedThree]

Looking through the rules, there are a couple that I think should perhaps be recategorised:

• Should assumed-size also be in bugprone?

Yes, I think that's reasonable

• deprecated-assumed-size-character feels like a style rule more than bugprone.

Hmm, maybe more like obsolescent than style. It's definitely stronger than a style recommendation

I believe assumed-size-character-intent will catch this syntax, though oddly we don't test for it.

Oh yes, that's a little awkward, though I'm not sure it's inaccurate.

I'm also looking at all of the rules for handling real types/kinds, and thinking perhaps it was a bad idea to split them between typing and precision. I'm considering moving all of them to a category floating-point/FL and deprecating the precision category entirely. While we're at it, I'd also like to move star-kind to the style category. Do you think that's something I should do as a separate PR?

If it's "just" rearranging, then maybe a separate PR?

Could we move things around before the merge? It's probably best for us to figure out exactly where things should be beforehand, so I think that should be the last thing we do in this PR.

Yes, that was exactly my intention too! :)

One weird side-effect is that the M category is now all preview rules, so selecting it will warn it does nothing without enabling preview mode.

I think that's okay, as I'd like to move a bunch of rules out of preview for the next release anyway.

The ones that were preview in the last release?

[LiamPattinson]

Hmm, maybe more like obsolescent than style. It's definitely stronger than a style recommendation

Good point, I hadn't spotted that this syntax is considered obsolescent in the standards.

---

If it's "just" rearranging, then maybe a separate PR?

Sure, I was just thinking that it's probably best to do all recategorising now while we're already at it.

---

The ones that were preview in the last release?

Yes, at least the uncontroversial ones. For example, I'd argue that missing-accessibility-statement should move out of preview, but not default-public-accessibility.

[ZedThree]

Sure, I was just thinking that it's probably best to do all recategorising now while we're already at it.

Sure, that's also reasonable!

[imciner2]

While I see the value in being able to say a whole category is in the default, it seems a bit of a stretch to label everything like this "bugprone", specifically rules like use-all and deprecated-assumed-size-character, and that categorization may just become more tortured as time goes on.

Perhaps it is worth instead slimming down the number of categories? I'll note that the open-catalog here https://github.com/codee-com/open-catalog only has 4 categories correctness, security, optimization and modernization, so perhaps moving to a model like that would be more sustainable and expandable than having all these specific categories.

As for defining defaults, having to move a rule to a new category just because it should be default-on seems to be a disruptive change, especially if the plan will be to add rules that are off by default and then transition them to being on after more usage and testing shows they work and are valuable.

[ZedThree]

While I see the value in being able to say a whole category is in the default, it seems a bit of a stretch to label everything like this "bugprone", specifically rules like use-all and deprecated-assumed-size-character, and that categorization may just become more tortured as time goes on.

My reasoning for use-all being bugprone is that it makes it much harder to detect undefined variables -- we don't do this yet though.

Yes, deprecated-assumed-size-character probably belongs in obsolescent or elsewhere.

Perhaps it is worth instead slimming down the number of categories? I'll note that the open-catalog here https://github.com/codee-com/open-catalog only has 4 categories correctness, security, optimization and modernization, so perhaps moving to a model like that would be more sustainable and expandable than having all these specific categories.

Yes, see #269 for some discussion on this. I do like modernisation as a category, perhaps replacing obsolescent, and a more natural home for things like deprecated-assumed-size-character.

As for defining defaults, having to move a rule to a new category just because it should be default-on seems to be a disruptive change, especially if the plan will be to add rules that are off by default and then transition them to being on after more usage and testing shows they work and are valuable.

It's less about requiring the default rules to be in a default-on category, but that maybe having a default-on category suggests the original categorisation of these particular rules wasn't quite correct, if that makes sense.

Any kind of reorganisation is going to be at least a little disruptive, but the rule redirects do actually take a lot of the pain out of that -- the old code still works for selecting and ignoring, and the user gets a warning about the new name.

However, I do think it would be best to try and consolidate any potential disruption, rather than spreading it out and having every update break something.

It does seem like people are generally happy to just turn off the individual rules they don't want, so it's possible we're worrying too much about this and could default to everything on!

[LiamPattinson]

As for defining defaults, having to move a rule to a new category just because it should be default-on seems to be a disruptive change, especially if the plan will be to add rules that are off by default and then transition them to being on after more usage and testing shows they work and are valuable.

I would hope that we wouldn't need to shuffle things around very often after this update, as this will result in there being a stricter hierarchy of rule categories:

• Tier 0, error: The code can't be parsed or something has gone wrong in Fortitude.
• Tier 1, bugprone, obsolescent: Something in the code is likely to cause errors, or a severely outdated language feature is in use that may be dropped in a future standard.
• Tier 2, typing, modules, style, etc: Rules that don't fit into the above categories, but there's still some way to promote best practices.

In my mind, by making bugprone and obsolescent the 'default' categories, we're deciding that only rules that fit those definitions should be on by default. No matter how well-tested and valuable a rule may be, if it only affects code style then we'll be leaving it switched off by default. Any new and untested rules that do fit those categories can still be hidden by the --preview flag while we work out any issues.

---

I do like modernisation as a category, perhaps replacing obsolescent

I'm not so sure, because right now obsolescent is for things that have been officially placed on the chopping block by the standards committee. I'd be up for adding a modernisation category that tells users, "There's a fancy new way to do X", but I see obsolescent as instead telling them, "This is not a drill, stop using X immediately".

---

I'm still not entirely on-board with this change, but here are a the pros and cons from where I'm standing:

Pros

• Much easier to document and explain the default behaviour.
• Less ambiguity regarding which rules should be on or off by default (and therefore hopefully fewer arguments in future, although I imagine use-all will catch a lot of complaints).
• Clarify the primary purpose of Fortitude: catch bugs that might be missed by your compiler. Enforcing modern code style is nice, but secondary.
• A stripped down set of default rules will make Fortitude less onerous to integrate into large projects, and will focus attention where it really matters. After running Fortitude and receiving 20,000 violations, most of which are pedantic style complaints, some users might just not bother. If they instead receive 50 violations, all of which are possible bugs or dangerous code patterns, users may be more inclined to act and continue using the tool.

Cons

• I wrote a lot of the rules that will be switched off by this update and I'm salty about it
• Tagging individual rules as Default or Optional would give more flexibility in deciding which rules we think are most important.
• Maybe we do care about promoting a particular code style? My original goal was to enforce recommendations from the Fortran-lang teaching pages, and a lot of those rules would be hidden by this.
• By hiding so many rules, Fortitude's impact as a educational tool may be decreased. I've seen a few people talking about how some particular rule taught them a better way to code, but if those rules don't fit the bugprone/obsolescent categories, a lot of users might just never hear about them.
• Adding/updating configuration files in response to this update will generally be a little annoying to everyone.

Overall, I'm leaning in favour of the recategorisation. My biggest worry is that a breaking change of this nature might be sufficiently annoying to scare off some of our current users, but I also worry that our current system is unsustainable, and the later we wait to make a large breaking change the bigger the problems will be.

@ZedThree Do you have anything else you would like to add that might swing it one way or the other?

[imciner2]

Tier 0, error: The code can't be parsed or something has gone wrong in Fortitude.
Tier 1, bugprone, obsolescent: Something in the code is likely to cause errors, or a severely outdated language feature is in use that may be dropped in a future standard.
Tier 2, typing, modules, style, etc: Rules that don't fit into the above categories, but there's still some way to promote best practices.

I guess I'll bike-shed the name then 😁, perhaps going with correctness would be better than bugprone.

I do like modernisation as a category, perhaps replacing obsolescent

I'm not so sure, because right now obsolescent is for things that have been officially placed on the chopping block by the standards committee. I'd be up for adding a modernisation category that tells users, "There's a fancy new way to do X", but I see obsolescent as instead telling them, "This is not a drill, stop using X immediately".

Yes, I can see separating the formally-deprecated/deleted items from just the "nice to do" modernizations being good (such as all the requested intrinsic replacements, or Appendix A of Modern Fortran Explained), so IMO two categories is reasonable for this.

• Tagging individual rules as Default or Optional would give more flexibility in deciding which rules we think are most important.
• Maybe we do care about promoting a particular code style? My original goal was to enforce recommendations from the Fortran-lang teaching pages, and a lot of those rules would be hidden by this.

I guess everything is going to be opinionated, but with the configuration files people can create custom rule sets that people could apply to their projects - so then the Fortran-lang projects could just distribute a config file that would activate all those rules that are recommendations from the pages. (I know my plan is to use a configuration file in my repo so I can use that in the CI and control the warnings generated).

• By hiding so many rules, Fortitude's impact as a educational tool may be decreased. I've seen a few people talking about how some particular rule taught them a better way to code, but if those rules don't fit the bugprone/obsolescent categories, a lot of users might just never hear about them.

I think this discoverability problem can be overcome by prominently documenting the --select ALL command line flag in the discussion about the default rulesets - e.g., telling people what the defaults are and then mentioning they can see the output of all rules using that flag.

• Adding/updating configuration files in response to this update will generally be a little annoying to everyone.

Yes, this is probably the pain point for growing, but it seems like a band-aid that needs to be ripped off eventually.

[ZedThree]

I think the outcome of this discussion and one @LiamPattinson and I have just had offline is that yes, we definitely want to have a more restricted subset of rules turned on by default, but that maybe we leave the reorganisation of categories for now, as there are some style rules that we want on by default (e.g. unnamed-end-statement). We had at least think of the re-categorisation as orthogonal to the default rules