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

Compiler emits Warning 59 on array assignment #7616

Closed
vicuna opened this issue Aug 31, 2017 · 7 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Aug 31, 2017

Original bug ID: 7616
Reporter: freyr
Status: resolved (set by @damiendoligez on 2017-10-02T12:16:42Z)
Resolution: fixed
Priority: normal
Severity: minor
Platform: linux x86_64
OS: fedora 26
OS Version: 4.12.8-300.fc26
Version: 4.04.2
Category: middle end (typedtree to clambda)
Monitored by: @gasche @hcarty

Bug description

Compiler emits warning 59 being emitted when I assign array to ref.
x := [||]; works fine, but x := CCArray.filter_map filter !x;
(CCArray.filter_map from containers package [1]) emits warning 59 (A potential assignment to a non-mutable value was detected)

  1. https://opam.ocaml.org/packages/containers/

Steps to reproduce

  1. compiler: 4.04.2 + flambda

  2. set -O3 flag

  3. compile a trivial program containing x := CCArray.filter_map filter !x;

Additional information

Trivial example with the corresponded Makefile is attached

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 1, 2017

Comment author: @Octachron

Interestingly, the warning is not triggered with 4.05.0+flambda but rears its head again with trunk.

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 4, 2017

Comment author: @xclerc

The problem is related to the definition of Simple_value_approx.is_definitely_immutable,
that considers Value_block approximations as immutable. The following commit fixes the
definition:

xclerc@ed522ba

(Note that the function is used only to trigger the warning.)

However, it would entail a small regression, as examplified by some of the tests present
in testsuite/tests/warnings/w59.opt_backend.ml. The warnings for the mutation of a
tuple would disappear. Indeed, as approximations for tuples and arrays are shared, I do not
see how flambda could generate a warning for the former but not for the latter.

It thus depends on whether we prefer false positives or false negative. One problem with
this particular false positive though is that the user cannot easily get rid of it (apart
from disabling the warning).

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 10, 2017

Comment author: @Octachron

False positives sound more benign than false negatives. Nevertheless, the flambda part of the manual currently only warns about false negatives for this warning. I think it would help users to at least clarify this point in the manual, and add a reference inside the warning message towards this extended explanation. It is also a bit unfortunate that this warning cannot be disabled locally with warning attributes.

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 11, 2017

Comment author: @xclerc

Indeed. Moreover, the manual proves me wrong: there is a way to
silence the warning. The reporter may amend filter_map [1] as
follows:

let filter_map f a =
let rec aux acc i =
if i = Array.length a
then (
let a' = Array.of_list acc in
reverse_in_place (Sys.opaque_identity a');
a'
) else match f a.(i) with
| None -> aux acc (i+1)
| Some x -> aux (x::acc) (i+1)
in aux [] 0

[1] https://github.com/c-cube/ocaml-containers/blob/master/src/core/CCArray.ml

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

Comment author: @lpw25

Haven't actually tested, but this PR should fix it:

#1339

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

Comment author: @xclerc

Tested - the PR fixes the problem on this instance.

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 2, 2017

Comment author: @damiendoligez

Fixed by #1339

@vicuna vicuna closed this Oct 2, 2017

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.