Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

per-type fragile match warning #7310

Closed
vicuna opened this issue Jul 31, 2016 · 12 comments
Closed

per-type fragile match warning #7310

vicuna opened this issue Jul 31, 2016 · 12 comments

Comments

@vicuna
Copy link

vicuna commented Jul 31, 2016

Original bug ID: 7310
Reporter: @jberdine
Status: acknowledged (set by @xavierleroy on 2017-02-25T15:13:00Z)
Resolution: open
Priority: normal
Severity: feature
Version: 4.03.0
Category: typing
Monitored by: @bobzhang @gasche bobot

Bug description

For code bases that have some/many fragile pattern matches, it would be very useful if it were possible to enable warning 4 on a per-type basis. For example, perhaps an attribute could be added to a type definition which would cause all fragile matches on that type to be reported.

The use case for this feature is when extending an existing sum type in a code base with fragile matches. Currently when a new constructor is added, warnings for non-exhaustive matches can be reported. This is a huge help. It would help even more if the fragile matches on the extended type could be reported as well. After inspecting all of them, the annotation could be removed.

Another use case is where a code base prohibits fragile matches only on some but not all types.

@vicuna
Copy link
Author

vicuna commented Aug 1, 2016

Comment author: @gasche

I think that this is an interesting feature request, and that a good interface (instead of a command-line option) would be an attribute on the type declaration, for example [@@fragile_pattern {error,warn,ignore,default}].

Besides 4 (fragile clauses), which other warnings would it make sense to enable on a per-declaration basis? I guess the ranges 44-45 (identifier shadowing), 40-42 (disambiguated constructors), 31-39 (unused declarations).

@vicuna
Copy link
Author

vicuna commented Aug 1, 2016

Comment author: @jberdine

I agree that the additional warnings you mention would also make sense. But to me, the next most valuable warning to make per-declaration would be 9 (Missing fields in a record pattern).

@vicuna
Copy link
Author

vicuna commented Aug 1, 2016

Comment author: @gasche

One thing that is unclear to me is what a good cascading semantics would be: which setting has precedence over which other setting?

In general the proper order is to favor the more specific setting over the less specific setting. Currently this means that command-line setting (and OCAMLRUNPARAM; and ocaml_compiler_internal_params before that) have less priority than local [@@warning] attributes. (Build-system-level settings are indistinguishable from command-line settings.)

While the priority of a command-line interface for this feature would be clear, declaration-specific attributes are delicate because they are along an orthogonal axis of specificity (one specific type vs. all types).

Should there be a way to disable a warning 9 fired by this type-specific feature from the command-line? Should the feature be able to silence a warning/error level explicitly set from the command-line?

One option would be to use a different warning number for this feature; warning 9 (general fragile pattern) could remain disabled by default, and the new warning (type-specific fragile pattern on enabled type) would be enabled by default.

@vicuna
Copy link
Author

vicuna commented Aug 1, 2016

Comment author: @Octachron

I would argue that this cascading semantic problem would be better solved by having parameterized warnings rather than duplicated warnings with slightly different semantics, e.g the warning 9 could have four levels:

  • strict: apply to all type, attributes can not silence a warning
  • standard(or implicit): apply to all types, attributes can silence warning
  • lenient(or explicit): apply only to types marked by an attribute
  • disabled: no warning.

@vicuna
Copy link
Author

vicuna commented Aug 8, 2016

Comment author: @jberdine

I am not sure how this proposed finer granularity for warning 9 is fundamentally different from the existing [@@warning] and [@@WarnError] attributes on expressions. That is, there is a difference between attributing the declaration instead of the uses, but in terms of priority, I think it could reasonably be treated the same. Perhaps I'm overlooking something more subtle?

@vicuna
Copy link
Author

vicuna commented Jul 19, 2017

Comment author: bobot

It seems that #1071 implements this request.

@github-actions
Copy link

github-actions bot commented May 9, 2020

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 9, 2020
@jberdine
Copy link
Contributor

jberdine commented May 9, 2020

The original motivation for this remains. There is a PR (#1071) implementing this, though it is stalled seemingly over the syntax of the warning specifications, and an open question that perhaps it implements too fine-grained a warning control mechanism.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 12, 2021
@gasche
Copy link
Member

gasche commented May 12, 2021

#1071, which fixes this issue, seems stalled because no one is willing to work on it for now. Any contribution is warmly welcome.

@gasche gasche removed the Stale label May 12, 2021
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@rr0gi
Copy link

rr0gi commented Jul 1, 2022

still want it 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants