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

Jump to bottom

On 4.05 warning 3 seems to be inherited #7580

Closed
vicuna opened this issue Jul 6, 2017 · 11 comments
Closed

On 4.05 warning 3 seems to be inherited #7580

vicuna opened this issue Jul 6, 2017 · 11 comments
Assignees
@dra27
Labels
bug high-priority typing

Comments

@vicuna
Copy link

vicuna commented Jul 6, 2017

Original bug ID: 7580
Reporter: bobot
Assigned to: @dra27
Status: resolved (set by @dra27 on 2017-07-06T13:01:19Z)
Resolution: not a bug
Priority: high
Severity: major
Version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: typing

Bug description

The warning for deprecation seems to be inherited when functions are defined as alias:

  include struct
    [@@@warning "-3"]
    let capitalize_ascii   = String.capitalize
    let uncapitalize_ascii = String.uncapitalize
    let uppercase_ascii    = String.uppercase
    let lowercase_ascii    = String.lowercase
  end

Jbuilder use this code for being compatible with different ocaml version, it works for older versions, but with 4.05, user of this module raise the warning 3.
@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @alainfrisch

This is probably #7444. The warning is now raised on the module coercion when the deprecation marker is removed. You can either eta-expand the function or disable the warning around the coercion (e.g. write a signature around the "struct" and disable the warning for the module definition).

Are you sure this is for 4.05, not 4.06/trunk, though?

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @dra27

I'm struggling to reproduce it (probably me...) - could you expand the example to be one which definitely produces the warning?

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: bobot

Could you add a * in the changelog?

- #7444, #1138: trigger deprecation warning when a "deprecated"
  attribute is hidden by signature coercion
  (Alain Frisch, report by bmillwood, review by Leo White)

Yes definitely 4.05. Th 1138#GPR is not in 4.05?

The reprocase can be the current jbuilder source (ocaml/dune#165 (comment)), but I'm going to try to create a smaller test case.

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @alainfrisch

I think it's implicit that warnings can become more aggressive across version, no?

No, #1138 should not be in 4.05. Perhaps it's something else, then.

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @dra27

I can confirm I'm also seeing it with jbuilder in 4.05.0+rc1

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: bobot

Okay, the code in jbuilder was faulty (use String.lowercase through an include in Import), I just don't understand why it does this in 4.05 and not just in trunk. I misread the reason of the error.

I still think that this changes should have a *, it just say that it is a change that can break existing programs. We can be explicit. Sorry for the noise.

The commit for jbuilder: ocaml/dune@1297f83

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @dra27

I'm just finishing a bisect to look further (the change was introduced after 4.04.0 was merged back into trunk but before 4.05 was branched) just in case it's also an OCaml bug (or a missing Changes entry).

We don't normally mark changes in warnings behaviour as breaking, I think - the idea is that one should never be running -warn-error in production, so we can't break anything.

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @dra27

The change appeared in 4da2b3 (4da2b3) - #875 (#875)

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @dra27

Oh - it's obvious. Jbuilder uses StringLabels and prior to this commit those functions were incorrectly not marked as deprecated!

@vicuna vicuna closed this as completed Jul 6, 2017
@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @alainfrisch

Thanks David! I think the mystery is fully resolved.

@vicuna
Copy link
Author

vicuna commented Jul 6, 2017

Comment author: @dra27

Indeed - I have clarified the entry for #875 in 4.05/trunk Changes to mention that missing deprecation tags were added.

@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dra27 dra27

Labels
bug high-priority typing
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dra27 @vicuna