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 7821 #1908

Merged
merged 1 commit into from Jul 16, 2018

Conversation

Projects
None yet
4 participants
@diml
Copy link
Member

commented Jul 16, 2018

#1010 introduced a heuristic to rewrite paths of the form X__Y to X.Y when they are the same. This is done when printing types and when compiling extension constructors by calling Printtyp.rewrite_double_underscore_paths. The latter wasn't protected via Printtyp.wrap_printing_env ~error:true and as a result can cause X.cmi to be loaded when compiling an extension constructor inside a compilation unit called X__Y. This PR fixes that by properly wrapping this call.

Unfortunately this is a serious problem as it breaks incremental builds of Base and probably other projects.

@trefis
Copy link
Contributor

left a comment

The fact that you pass ~error:true even though this call has nothing to do with the printing of an error smells fishy to me.

Do you really have to do the rewriting in that case?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

We already merged another wrap_printing_env fix in #1893. Does this other one also have the potential to break parallel builds? Should we cherry-pick it in the 4.07 maintenance branch?

@Octachron the fact that those failures are more dangerous than we thought makes me even more convinced that a type-level approach to protect the wrap_printing API would be valuable -- for others, see suggestions in #1893.

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@trefis, yes, otherwise Stdlib.Queue.Empty would be printed as Stdlib__Queue.Empty

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

I'm definitely in favor of a type-level approach

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@gasche , fortunately, the bug in #1893 can only affect 4.08 since it relies on the more complex printing of identifiers introduced in #1120 and without this change string_of_path does not depend on the printing environment.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@trefis, yes, otherwise Stdlib.Queue.Empty would be printed as Stdlib__Queue.Empty

I see.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

Should we cherry-pick it in the 4.07 maintenance branch?

I think so.

@gasche

gasche approved these changes Jul 16, 2018

Copy link
Member

left a comment

The patch is clearly safe and behaviour-preserving. Approved.

Is it possible to write useful tests for this kind of issues? How do we check for a missing wrap_printing_env? Would it be possible to configure the global constants in such a way that running the dangerous functions outside of a wrap results in an error, and use this mode on a CI machine?

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

Another random complaint: I can't see why one would ever want to pass ~error:false to wrap_printing_env. There are a few examples in the code base, but they don't make sense to me.

@diml diml merged commit 48edf92 into ocaml:trunk Jul 16, 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 16, 2018

(@diml: you should cherry-pick as well)

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

It should be possible to write tests for this, however I'm not 100% how to set them up with the current testsuite. BTW, we have been using cram-like tests in dune to test this kind of behavior and it has proven to be extremely productive. Adding regression tests for this kind of cases is extremely easy. I think the compiler would greatly benefit from such tests as well.

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@gasche done

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

Would you have a pointer to those cram-style tests?

@Octachron I'm thinking of giving the typed-API for wrap_* a go someday this week, would I be walking on your toes?

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@trefis agreed. At first I didn't even understand the link the name of the argument and the behavior.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@gasche : please go ahead, I don't have enough toes for that this week.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

Alright, I'll submit a PR to always turn loading of cmis off when under wrap_printing_env.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

@trefis and implement the type-based approach? I'm also short on toes -- I have 11 POPL papers to review in a few weeks, and only 10 toes.

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@gasche, the initial idea comes from the python world: https://bitheap.org/cram/. In dune we have our own OCaml implementation and @samoht also did a better packaged version: https://github.com/realworldocaml/craml. The idea is the same as expectation tests: you write shell commands followed by their expected output.

Here is one example taken from the dune test suite:

In dune files
-------------

Duplicating a field in a dune file is an error:

  $ dune build --root dune
  File "dune", line 4, characters 1-20:
  Error: Field "action" is present too many times
  [1]

In jbuild files
---------------

For backward compatibility, it is only a warning in jbuild files:

  $ dune build --root jbuild
  File "jbuild", line 4, characters 2-21:
  Warning: Field "action" is present several times, previous occurrences are ignored.
  Entering directory 'jbuild'
  bar

Line starting with $ (two spaces + dollar sign) are shell commands. Lines starting with (two spaces) are their expected output and the other lines are comments.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@trefis and implement the type-based approach?

No.
I'm proposing a one line change, and you're trying to push me to do a fairly substantial rewrite of Printtyp; do you realize how hard it is to answer you politely? :)

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

I'll give a shot at the the type-based approach

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

Nevermind, I remember why the ~error flag is here now, cf. #1657 .
Then perhaps the name should be changed to forbid_loading_cmis or something, but I can't quite be bothered, so as I was saying: "nevermind".

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@diml : you merged without updating the Changes entry (you left "GPR#..." in there). Could you please push a fix?

@diml

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

oops, fixed

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

@gasche it seems clear to me that this should be cherry picked to 4.07: the bug is very annoying, and the fix "clearly safe".
If you want I can move the changelog entry to the right section (adding you as a reviewer which at the same time) and then cherry-pick on 4.07.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Jérémie did cherry-pick in 4.07 already, but he forgot to adjust the Changes entry position in trunk. Please feel free to do it.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

Ah indeed, I was looking at my local 4.07 not origin/4.07...
Will do.

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.