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

proposal: enable warning 6 ([labels-omitted]) by default #10118

Closed
gasche opened this issue Jan 4, 2021 · 5 comments
Closed

proposal: enable warning 6 ([labels-omitted]) by default #10118

gasche opened this issue Jan 4, 2021 · 5 comments

Comments

@gasche
Copy link
Member

gasche commented Jan 4, 2021

OCaml understands full unlabelled application of labelled functions, but emits warning 6 ([labels-omitted]). This warning is not enabled by default -- but it is part of the Dune default warnings, so in practice it is the default for a large part of the userbase today.

I believe that the reason for not enabling it by default comes from a "transitory" period when labelled functions were introduced. I don't see a good reason today to keep encouraging this programming style.

On the other hand, when an API designer uses label, they generally consider that their users will have to use them; it is used to clarify caller-side code, and in particular to avoid the programming error of mistakenly swapping arguments of compatible type. Encouraging non-labelled applications weakens these benefits.

This suggestion originates from #10105, which considers replacing a non-labelled function with a labelled function using a different argument order. Currently (with warning 6 disabled by default), non-labelled callsites will keep compiling just fine, even if they forgot to fix the argument order, leading to silent bugs introduced by innocuous refactorings.

(* Declaration, before: *)
let starts_with str sub = String.sub str 0 (String.length sub)

(* Declaration, after: *)
let starts_with ~sub str = String.sub str 0 (String.length sub)

(* This changes silently breaks this code *)
let is_prompt line = starts_with line "#"
@gasche
Copy link
Member Author

gasche commented Jan 4, 2021

(At some point we should probably review the warnings enabled by default in the compiler, and with Dune, and see if we could/should try to remove some of the differences. It's not so good to have a large discrepancy between observed warnings across the two systems.)

@xavierleroy
Copy link
Contributor

I'm not an expert on this topic but I'm broadly in favor of having this warning "on" by default, and perhaps move towards stricter handling of labels in the future.

If I remember correctly (otherwise @garrigue will correct me), when labeled arguments from OLabl were added to OCaml, we first hoped that labels at application sites could be kept entirely optional, so that the stdlib function could be labeled without breaking existing code. This turned out to be possible for full applications (hence the support we have today) but not for partial applications (hence the two versions labeled/unlabeled for stdlib modules).

Nowadays, I think that labels on a function definition should be honored at all call sites. It is clearer and simpler this way.

@dbuenzli
Copy link
Contributor

dbuenzli commented Jan 8, 2021

(At some point we should probably review the warnings enabled by default in the compiler, and with Dune, and see if we could/should try to remove some of the differences. It's not so good to have a large discrepancy between observed warnings across the two systems.)

I think that would be an excellent idea, here's a data point.

@gasche
Copy link
Member Author

gasche commented Jan 9, 2021

It's not so easy to get feedback on this proposal, so I think that I will turn this issue into a PR. Somehow people react more consistently when we are one click away from the thing they don't like.

@gasche
Copy link
Member Author

gasche commented Jan 11, 2021

#10140 is the PR with the change. I propose to migrate further discussion there. Thanks!

@gasche gasche closed this as completed Jan 11, 2021
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

3 participants