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

Fix usage warnings with no mli file #1358

Merged
merged 3 commits into from Mar 14, 2018

Conversation

Projects
None yet
4 participants
@lpw25
Copy link
Contributor

lpw25 commented Sep 20, 2017

The following .ml file:

module Q (M : sig type t end) = struct end

with the following .mli file:

module Q : functor (M : sig type t end) -> sig  end

helpfully emits the warning:

File "unused.ml", line 1, characters 18-24:
Warning 34: unused type t.

However, if the .mli file is removed then the warning disappears, even though this is the .mli file that ocamlc -i generates for the .ml file.

The problem comes from the inclusion check between the module and its interface. An inclusion check like:

Includemod.modtypes ~loc env a b

will mark the definitions occurring positively (covariantly) in a and negatively (contravariantly) in b. In most cases this is correct, but when checking an .ml file against its interface we only really want to mark the definitions occurring positively in a.

There already exists Env.implicit_coercion which is used for dealing with implicit coercions. However, this disables all marking in an inclusion, not just the negative marking.

This PR removes Env.implicit_coercion and adds parameters to the Includemod functions with type:

(** Type describing which arguments of an inclusion to consider as used
    for the usage warnings. [Mark_both] is the default. *)
type mark =
  | Mark_both
      (** Mark definitions used from both arguments *)
  | Mark_positive
      (** Mark definitions used from the positive (first) argument *)
  | Mark_negative
      (** Mark definitions used from the negative (second) argument *)
  | Mark_neither
      (** Do not mark definitions used from either argument *)

Mark_both is used for most inclusions, Mark_neither is used for implicit coercions, and Mark_positive is used when checking an .ml file against its interface. (Mark_negative is used within Includemod and is exposed in the interface for completeness).

@lpw25 lpw25 force-pushed the lpw25:fix-usage-warnings-with-no-mli-file branch from c6fe543 to 2780cd2 Sep 20, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Sep 20, 2017

This seems cleaner and more correct than the current approach. The code looks good.

Can you add a test for the no-mli case?

@gasche gasche closed this Sep 27, 2017

@gasche gasche reopened this Sep 27, 2017

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Oct 4, 2017

Ok. I've added a test for the no-mli case.

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Oct 9, 2017

@alainfrisch alainfrisch self-assigned this Feb 28, 2018

@@ -0,0 +1,2 @@
File "w32b.ml", line 1, characters 18-24:

This comment has been minimized.

@alainfrisch

alainfrisch Feb 28, 2018

Contributor

This should be "line 3", not "line 1".

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 28, 2018

@lpw25 Could you fix the test as reported, and merge/rebase with trunk?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Mar 12, 2018

Ping @lpw25

@lpw25 lpw25 force-pushed the lpw25:fix-usage-warnings-with-no-mli-file branch from 8d4055e to 6811e62 Mar 13, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Mar 13, 2018

Rebased, test fixed and Changes entry added.

@alainfrisch alainfrisch merged commit bce9d4b into ocaml:trunk Mar 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.