Rule idea: forbid elements by name

[jfmengels]

What the rule should do:

Report usages of specific values/functions/types/modules by name.

What problems does it solve:

Making it much easier to forbid the usage of certain elements, lowering the entry to use elm-review.

Example of things the rule would report:

Given configuration like

RuleNameTBD.rule
  [ { value = "Html.img"
    , message = "Use Accessibility.img or Accessibility.decorativeImg instead of Html.img"
    , details = [ "...some explanations as to why..." ]
    }
  ]

then this rule would report references to Html.img with the message and details defined above.

When (not) to enable this rule:

When you don't have specific things you want to forbid or move away from.

I am looking for:

I have plenty of questions around this rule, and could use a lot of feedback!

1. Collision with NoDeprecated

The NoDeprecated rule in this same package has quite a bit of overlap with this proposal. For instance, you can forbid a function defined in your codebase by either tagging it as deprecated, or by using this rule by explicitly targeting the function by name.

Which one to use becomes a bit of an unclear decision. We can probably merge the two ideas, but then it shouldn't be called NoDeprecated. There is definitely some overlap, but it's also not a perfect overlap.

2. Need for exceptions

A common use-case for this rule is to forbid something in favor of a custom solution. For instance, you could define a new Button module and then forbid the Html.button function. But you would still want to support the usage of Html.button in this one module. So each restriction likely needs to be configured with exceptions. Except that you can't use the existing Review.Rule.ignoreErrorsFor* functions as that would make the exceptions apply to unrelated forbidden elements.

Unless you use the rule multiple times, which is possible (but potentially confusing for people who come from ESLint where that's not possible because redefining a rule overrides it).

3. Suppressions

One thing that I've noticed with NoDeprecated is that when you're using elm-review's suppression system (which the rule is pretty much meant to), you can get a large amount of suppressions (we have over a 1000 at work), and it can be a bit hard to figure out which deprecated functions are responsible for most suppressed usages. I have a proof of concept to use --extract to help give that insight, which could help a bit.

I'm thinking that if we have one rule with plenty of these restrictions and we suppress that, then once again it would be hard to tell which functions are still used a lot and which ones don't have any issues.

4. There probably needs quite a few configuration options to forbid functions vs types (and especially record type constructors) vs modules vs dependencies vs other things. The proposed API above only works for functions and/or types, but depending on the scope, we might want more.
   These could also be split into multiple new rules.

5. Maybe I'm overthinking or over-engineering this rule, and it would be nice to have a simpler rule (or way of writing this rule), for instance that would only forbid a single element. But that then becomes annoying if you have plenty of related functions you want to forbid.

6. I also need a good rule name.

I'm thinking NoForbiddenReferences or something. I do find the double negation a bit confusing though, so maybe only ForbiddenReferences.
ESLint has multiple "no-restricted-*" rules, like "no-restrict-syntax" and "no-restricted-globals" which are close. But I have found the name very confusing for years.

That said, if we decide to limit this rule to forbidding a single element, then the rule name should likely be configurable by the user of the rule, as that is a somewhat important part of an elm-review error. But the module containing this rule would still need to be named 😄

7. I'm thinking I could in the documentation of the rule show the entire implementation of the rule, so that if someone wants to extend the rule a bit, then they can easily do so with that example as the source code. Does that sound like a good idea?

Thoughts and ideas welcome 🙏

[HilcoWijbenga]

The extremely nice thing about NoDeprecated is that it (also) works with an annotation. So it is really clear in the code where the problem is.

In general, some form of annotation to suppress instead of excluding an entire file would be much more maintainable and granular. Obviously, it would also be more complicated to implement in elm-review. (And I am not claiming that everything can be achieved with just annotations.)

[That was probably not exactly the sort of feedback you were looking for?]

[wolfadex]

This reminds me a lot of NeoVier/elm-review-no-function-outside-of-modules, but with the added bonus details about why it's forbidden. I did a very minor experiment with a similar rule internally, though much more limited and the extra details was something I added as well.

1. I think the overlap with NoDeprecated is ok since that rule also checks for @deprecated comments, which feels fairly distinct. I could, like you pointed out, see deprecating NoDeprecated (pun not intended) in the future for a more generic comment based rule.

2. I like the general approach of NeoVier/elm-review-no-function-outside-of-modules where instead of a blanket ban on a function you specify only the modules it's allowed to be used in.

3. This sound really nice. At Vendr we're nowhere near that many suppressions. We only have 34 currently, but it'd be nice to have some additional information/tooling/something to be able to dig deeper into that data and incrementally work through them. Unlike the common // linter-ignore approach of other tooling, the suppression approach is a little more challenging to search for. I don't want to stray to far from the topic though with this.

4. ForbidUseOf or ForbidUseOfOutsideOfModule, though that last one feels like a mouth full 😄. ForbiddenReferences I think sounds better than my 2nd suggestion.

5. Not sure this is necessary as they could also read the code, no?

[HilcoWijbenga]

As far as names go, maybe RestrictSyntax (or RestrictedSyntax) since that seems to be what we are doing?

[wolfadex]

I think I'd avoid references to "restricting syntax" like the eslint rule since this isn't restricting syntax itself but instead restricting named values (potentially types). Maybe RestrictedValues and a sister rule of RestrictedTypes? Though I still like ForbiddenReferences more than those.

[jfmengels]

Thanks for the input so far 🙏

The extremely nice thing about NoDeprecated is that it (also) works with an annotation. So it is really clear in the code where the problem is.

In practice we like to rename functions/modules, because that makes it clear something shouldn't be used, even without the use elm-review (during code review or when looking over some code). But yes, it's nice that it does both :)

In general, some form of annotation to suppress instead of excluding an entire file would be much more maintainable and granular. Obviously, it would also be more complicated to implement in elm-review. (And I am not claiming that everything can be achieved with just annotations.)

It's actually do-able in practice. elm-review-performance does that for its (only) rule. But instead of having it be part of the framework, its up to the rule (and I'm so far happy it's not spreading too much to other rules).

So this would definitely be an option for this rule. I'm not sure where it's better to write the exceptions though. When in doubt, I'd still do it in the configuration if both are possible.

[That was probably not exactly the sort of feedback you were looking for?]

As long as we don't stray too much off the topic, I'm fine with it. Also, your second point did bring an interesting possibility for the rule that could be exception.

This reminds me a lot of NeoVier/elm-review-no-function-outside-of-modules, but with the added bonus details about why it's forbidden. I did a very minor experiment with a similar rule internally, though much more limited and the extra details was something I added as well.

Ah yes, I forgot to check whether this already existed. I know it was created at some companies but forgot to check the registry. (something something discovery or rules could be improved)

The package doesn't work anymore, but it was republished under @henriquecbuss's name (hello relevant person 👋) after a username change. I'd also be fine with using that rule, maybe pushing for some improvements like message/details customization

And maybe a rename. I'm not sure it's the best one, especially as you can configure the rule to not allow a reference anywhere (which is great but I would not have expected that from the name). An exception through this method (and especially the module name which is nicer than the path) sounds like a good idea though.

@henriquecbuss how has been your experience with using the rule so far?

we're nowhere near that many suppressions. We only have 34 currently, but it'd be nice to have some additional information/tooling/something to be able to dig deeper into that data and incrementally work through them.

@wolfadex I'd be happy to help you out with that. Maybe let's discuss that in a separate issue or on Slack. Maybe there are a few "tricks" that is not widely known (like deleting a line in the suppressed file, that's okay) to make working with this easer. But yeah, separate topic.

ForbidUseOfOutsideOfModule

As I said, this confused me. I think it's better to call it like what I suggested or like ForbidUseOf. It can be confusing in the other way (that you can't have exceptions for it), but maybe the short description of the rule can make that clearer very quickly. For instance "Reports uses of configurable functions, with configurable exceptions." (it doesn't read nice because of the repetition, but you get the idea).

Not sure this is necessary as they could also read the code, no?

It's definitely not necessary, but it might be the little push people need to get started with custom rules. As long as it doesn't take a too prominent part of the documenation, this should be fine. This could also just be a link with explanations on how to extend this code.

[gampleman]

My instinct (FWIW) would be to have a rule that takes a single record and relies on repeating the rule multiple times to get the configuration you want. Then the name of the rule can be dynamic (spitball ForbidReferences.no "Foo.bar" ... => NoFooBar as the rule name?) and you gain plenty of flexibility with ignoreErrorsFor*.

I don't think there is much overlap with NoDeprecated from a user point of view (I imagine the implementation would be very similar). But the use case for NoDeprecated is to do rolling upgrades and the automatic detection of @deprecated really can help for that, as those doc comments can "automatically" come from third party dependencies and/or codegen. The use case for this rule is to enforce good practices around using better options that have been picked in the codebase, such as higher level APIs etc. For instance the Html.img vs Accessibility.img doesn't mean Html.img is deprecated (in fact I'd imagine Accessibility.img is implemented on top of Html.img), just that there is a safer/better/higher-level API available.

+1 on putting the implementation (perhaps slightly simplified even) into the docs if it's simple enough. It's a really basic rule (it's definitely the first rule we've built at work).

[henriquecbuss]

I really like the idea behind this kind of rule. I also really agree that my rule could be improved.

I've never really loved the name - it's too long, hard to remember, and, as you said, doesn't really convey everything that the rule can do. But it's hard to name it!

I think there are some ways we could improve the rule configuration:

• The rule's argument is of type List ( List String, List String ). Can you tell what this means? I can't
• Maybe we could validate the allowed options. For example, if we forbid a function foo from being used, except in module Bar, should we warn the user if foo is not used inside Bar, or if Bar doesn't exist?
• Should we check if the forbidden functions don't exist anymore? At work, we've had a few configurations of this rule that were outdated, and this is kind of hard to spot

I like the idea of providing custom error messages. My rule also shows what modules are allowed in the error messages - I think this is good for discoverability.

While developing the rule I went back and forth between relying on having multiple NoFunctionOutsideOfModules.rule ... |> ignoreErrorsFor* .... I think this is a decent approach, but can be confusing for people who are new to elm-review and want to set up this rule (although the current configuration is also confusing).

Just to have some data points, our most common use cases for this rule are:

• Wrapping over existing function (like the Button example, which wraps over Html.button)
• Having "private packages" inside our repo - a nicely contained interface, that can be split into multiple "internal" modules (just like some packages have Foo.Internal.* modules)