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

warning 40: fix missing wrap_printing_env #1893

Merged
merged 1 commit into from Jul 11, 2018

Conversation

Projects
None yet
2 participants
@Octachron
Copy link
Contributor

commented Jul 10, 2018

This PR fixes a missing call to wrap_printing_env in #1120 that was causing spurious warnings 49 (missing cmi) when a warning 40 (name not in scope) was triggered and the compiler was called with -no-alias-deps.

More precisely, the bug path in

module Foo = struct
  type info = { doc : unit }
  type t = { info : info }
end
let add_extra_info arg = arg.Foo.info.doc

Warning 40: doc was selected from type Foo.info.
...
Warning 49: no cmi file was found in path for module Foo

was:

  1. When printing Foo.info, Printtyp.string_of_path checks that the module Foo is the one in scope in the printing environment.
  2. But due to the missing call to wrap_print_env, Foo is not defined in the printing env
  3. Thus, Env.lookup_module looks for foo.cmi
  4. When -no-alias-deps is defined and Env.lookup_module is called with ~load:false
    Env.check_pers_struct is called
  5. Env.check_pers_struct raises warning 49 if no cmi exists for module Foo.

The change in typecore.ml fixes the first and second point: Printtyp.wrap_module_env ~error:true both sets the printing env and disables cmi lookup. (The second new test example illustrate why it is useful to update the printing env rather than simply disable cmi lookup with Env.without_cmi). Similarly, the change in printtyp.ml adjust the lookup options to avoid the third and fourth point altogether.

This fix addresses the first and third point reported by @sliquister . However, the warning message is still computed even if the warning is not active since I think that avoiding computation for disabled warnings sounds PR worthy by itself.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

I think it would be nice to have a Changes entry -- this is only in trunk, but this is still a bug that was reported.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

(The following are more open-ended question that can wait for a later P.R.)

There are other unprotected uses of Printtyp, in includemod and typeclass for example.

Which of the calls should be protected by by this wrap_printing_env function? How do you make a distinction between functions that are meant to be called within a wrap_printing_env context, and which should use the wrapper themselves?

I wonder if it would be possible to create a specific type and argument-passing discipline to represent this: if a function needs to be called from withing a "printing_env" (in particular, the relevant functions of the Printtyp module, but also the functions from other modules defined on top of the Printtype primitive), take a phantom printing_env argument. And change the type of wrap_printing_env to (printing_env -> 'a) -> 'a instead of (unit -> 'a) -> 'a.

@gasche

gasche approved these changes Jul 10, 2018

@Octachron Octachron force-pushed the Octachron:deferred_printing_warning_40_fix branch from ed4966c to 1c00e98 Jul 11, 2018

warning 40: fix missing wrap_printing_env
and amend lookup options in typing/printtyp.ml

@Octachron Octachron force-pushed the Octachron:deferred_printing_warning_40_fix branch from 1c00e98 to 5251865 Jul 11, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

I have added a change entry, while precising that the bug was only on trunk to avoid some potential misunderstanding.

There are other unprotected uses of Printtyp, in includemod and typeclass for example.

In includemod the unprotected uses of Printtyp are done in print_coercion which is a debugging function (which only use is currently commented out), for typeclass the only unprotected use of Printtyp are call to Printtyp.string_of_label which is a simple function that does not need any context. ( I have checked that all calls in typing and the toplevel are correctly protected).

The general rule is that only functions that go through the outcome tree representation require to set up the printing context before calling them. It should indeed be possible to manifest this requirement in the types of those functions.

Another complementary option might be to reorganize the interface of Printtyp to make the distinction between simple and outcome-tree based functions more apparent (with a Simple submodule for instance?).

@gasche gasche merged commit 309c8d4 into ocaml:trunk Jul 11, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Thanks!

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

(and thanks @sliquister for the swift testing and reporting.)

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.