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

Warn on literal patterns found anywhere in a constructor's arguments. #2133

Merged
merged 5 commits into from Nov 7, 2018

Conversation

Projects
None yet
2 participants
@yallop
Copy link
Member

commented Nov 3, 2018

The attribute @ocaml.warn_on_literal_pattern enables a warning for code that uses a literal pattern to match a constructor's arguments.

# exception E of string [@ocaml.warn_on_literal_pattern];;
exception E of string
# try () with E "foo" -> ();;
Characters 14-19:
  try () with E "foo" -> ();;
                ^^^^^
Warning 52: Code should not depend on the actual values of
this constructor's arguments. They are only for information
and may change in future versions. (See manual section 9.5)
- : unit = ()

Predefined exceptions, such as Failure, are marked with the attribute to dissuade people writing exception-handling code from relying on the value of the exception's arguments:

# try () with Failure "foo" -> ();;
Line 1, characters 20-25:
1 | try () with Failure "foo" -> ();;
                        ^^^^^
Warning 52: Code should not depend on the actual values of
this constructor's arguments. They are only for information
and may change in future versions. (See manual section 9.5)
- : unit = ()

However, the warning is currently arbitrarily restricted to constructors of a single non-tuple argument, and so no warning is emitted for more complex patterns:

# try "" with Failure ("foo" as s) -> s;;
- : string = ""
# try () with Match_failure ("foo", _, _) -> ();;
- : unit = ()
# try () with Failure ("foo" : string ) -> ();;
- : unit = ()

This PR extends the definition of 'literal pattern' so that warnings are emitted in these cases, too.

#254 has some previous discussion of the attribute/warning, and the possibility of extending them beyond a single constructor. @gasche's comment is particularly relevant:

#Following #379, I have regrets about the current state of the this PR. Due to implementation limitations, we only support constructors with a single argument, but using the attribute on constructors with several arguments will just silently make it meaningless and not warn the user in any way. This is very bad usability.

@yallop yallop force-pushed the yallop:warn-on-literal-full branch from c641d1d to e86a150 Nov 3, 2018

@gasche
Copy link
Member

left a comment

Thanks for working on this!

I think that the behavior with the PR is better than without. The design is still not fully satisfying however: having the attribute on the constructor (rather than on a parameter or possibly sub-types of the parameter) forces a very coarse-grained behavior, with some risk of users being unable to express their intent on which part they would like to avoid warnings about.

Could you maybe add a couple examples test-cases that did not warn before and now correctly warn with the PR?

Show resolved Hide resolved typing/typecore.ml Outdated
Show resolved Hide resolved typing/typecore.ml Outdated

@yallop yallop force-pushed the yallop:warn-on-literal-full branch from e86a150 to 3d18694 Nov 5, 2018

@yallop

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

@gasche:

Could you maybe add a couple examples test-cases that did not warn before and now correctly warn with the PR?

Good idea. 3d18694 adds a few such cases.

@gasche

gasche approved these changes Nov 5, 2018

Copy link
Member

left a comment

I approve of the PR, but:

  • you must add a Changes entry (and possibly check in the description of the warning in the manual whether it needs an update), and

  • I think that the testsuite file that you touched would be much nicer with expect-style test, it would be nice if you updated it to this style. (I think it's a general good-hygiene principle to update each file we touch to gradually migrate.)

@yallop

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

@gasche:

you must add a Changes entry

Done (5f14aa1).

(and possibly check in the description of the warning in the manual whether it needs an update)

Also done (894df10). The existing text is mostly okay, but I added a short paragraph to make it clear that the warning applies where the pattern is not a literal itself, but contains literals as sub-patterns.

I think that the testsuite file that you touched would be much nicer with expect-style test, it would be nice if you updated it to this style.

Also done (5521787).

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Thanks! This is good to go if/when CI passes. If I were to forget to come check it again, please (anyone) feel free to ping or merge.

@yallop

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2018

Thanks, @gasche. CI is passing.

@gasche gasche merged commit d5e75fb into ocaml:trunk Nov 7, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yallop yallop deleted the yallop:warn-on-literal-full branch Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.