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 fatal error from Parmatch.get_type_path #1425

Closed
wants to merge 3 commits into
base: 4.06
from

Conversation

Projects
None yet
7 participants
@trefis
Copy link
Contributor

trefis commented Oct 12, 2017

I am extracting a fix @vprevosto proposed in another GPR he submitted a few releases ago that hasn't been merged yet.

This is a fix for MPR#6394, however I chose to insert the changelog entry with this GPR as key and title because it's less obvious (from its title) to see how MPR is related.
But I'd be happy to change that.

Also, I took the liberty of rewriting a tiny bit Virgile's patch and adding some comments, I hope no one will mind.

All the commits should be squashed before merging.

fix long-standing bug with warning 4 and recursive module
The same issue that was reported for typecore.ml in PR#6394 did occur in
fragile pattern-matching detection code. Fix is similar: accept the fact that
in presence of recursive module some incoherence may happen.
@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Oct 12, 2017

This is extracted from #1071. The fix looks good to me, inasmuch as I can see how the change works.

It would be good to include a test: it didn't look like the original MPRincluded its test case, so why not:

diff --git a/testsuite/tests/typing-modules/pr6394.ml b/testsuite/tests/typing-modules/pr6394.ml
new file mode 100644
index 000000000..95f4f627c
--- /dev/null
+++ b/testsuite/tests/typing-modules/pr6394.ml
@@ -0,0 +1,19 @@
+[@@@ ocaml.warning "+4"]
+module rec X : sig
+  type t = int * bool
+end = struct
+  type t = A | B
+  let f = function A | B -> 0
+end;;
+[%%expect{|
+Line _, characters 6-63:
+Error: Signature mismatch:
+       Modules do not match:
+         sig type t = X.t = A | B val f : t -> int end
+       is not included in
+         sig type t = int * bool end
+       Type declarations do not match:
+         type t = X.t = A | B
+       is not included in
+         type t = int * bool
+|}];;

@trefis trefis force-pushed the trefis:parmatch-old-bug branch from d1b4de4 to bc78c13 Oct 12, 2017

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Oct 12, 2017

Added the test and added you to the list of reviewers :)

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Oct 12, 2017

If this is a fix for MPR#6394 then you should mention it in the Changes entry.

But I don't understand why MPR#6394 was closed as fixed, but still needs fixing. Could you explain?

@damiendoligez damiendoligez added the bug label Oct 12, 2017

@damiendoligez damiendoligez added this to the 4.06.0 milestone Oct 12, 2017

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Oct 12, 2017

But I don't understand why MPR#6394 was closed as fixed, but still needs fixing. Could you explain?

Because MPR#6394 only noticed the issue in typecore, presumably because warning 4 wasn't enabled when people noticed/tested/reproduced the issue.

I'll mention the MPR in the changelog.

@trefis trefis force-pushed the trefis:parmatch-old-bug branch from bc78c13 to d083397 Oct 12, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 12, 2017

I haven't looked at it yet. Regarding 4.06: what is the riskyness of the patch?

In theory, removing an assert false could result in compiler runs that previously failed and now generate wrong code (which is strictly worse). How confident are we that the code hitting this case will indeed fail later down the chain, is it easy to verify or does it rely on complex invariants?

@vprevosto

This comment has been minimized.

Copy link
Contributor

vprevosto commented Oct 12, 2017

@gasche I'd tend to claim that if the removal of assert false was OK for MPR#6394 (where the assert false was removed from the type checker), then this is also the case here (we are only checking whether a warning should be emitted or not).

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Oct 13, 2017

@maranget @xavierleroy opinions? reviews?

@trefis trefis force-pushed the trefis:parmatch-old-bug branch from d083397 to 004a964 Oct 13, 2017

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Oct 17, 2017

@maranget @xavierleroy opinions? reviews?

@gasche is probably another sensible choice for reviewing this. (i.e. "ping")

Btw, I agree with @vprevosto : considering the way MPR#6394 was fixed, we're not generating more wrong than if the warning was simply disabled.
The question could then apply to the fix of MPR#6394 : « Was this the "right" fix? Are recursive modules the only way to break this invariant? Are we sure there is no other way to break this which would also avoid any later checks? ».
I don't have the answer (and yes, it would be better if the fix was different and the question didn't need to be asked) but @garrigue seemed to think it was OK. So unless he changed his mind and we decide to revert his fix, I think this change should go into 4.06.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 18, 2017

(There is no chance I can look at it this week.) @garrigue, do you approve this PR?

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Oct 19, 2017

First, it's not clear to me how this change fixes MPR#6394. Indeed, it fixes a different symptom. The two are related, but there is nothing in MPR#6394 that refers to the problem fixed here. I would either remove the reference to MPR#6394 from the log, or explain this new problem in the MPR.

Then the original fix to MPR#6394 just protected itself against missing definitions in the environment, without touching the calling code. There is a comment about having a type error later, but it seems to me that the intrinsic correctness of the patch didn't depend on it: the assertion was removed because what I thought was dead code was not so actually. Here I see repeatedly comments that we can remove work because there will be a type error anyway. I'm not sure we can rely on it. On the other hand, it seems that this function is only called in checks of fragility, which are not related to compilation anyway, so the worse we could have is a missing warning in an unexpected corner case. I think I can live with that.

Conclusion, for me it's fine, but @maranget would know better about fragility.

@vprevosto

This comment has been minimized.

Copy link
Contributor

vprevosto commented Oct 19, 2017

I'd argue that the symptom is very similar to MPR#6394: something that was thought to be dead code was not so, namely the fact that looking up in the environment for a type involved in pattern-matching and do not get a Tconstr in return. In the context of PR #1071, I uncovered the issue on the test case that was added for MPR#6394, because in this setting the code that checks for fragile pattern matching gets executed regardless of the status of warning 4, as the decision on whether to emit a message is taken on a per-type basis (incidentally, @dra27 this is why there is no additional test in #1071: the existing ones are already exercising the corresponding branch). That said, I agree that the comment on type error is probably misleading (even though I'm not sure how a well-typed program could introduce such incoherence, but I do not know much about OCaml's type system and in any case, it is not the job of parmatch.ml to detect a type error), and it is probably a good idea to remove it.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Oct 24, 2017

As I understand, we're getting close to the release so I'm reviving this thread one last time:

I agree with the two messages above: this GPR fixes a different symptom than the one reported in MPR#6394.
The underlying issue being that inconsistent equations have been added to the environment. That is why @vprevosto and I see this issue as related to MPR#6394, and might have been a bit confused by @garrigue's answer (which seems to imply he only cares about the symptom).

The key difference between Jacques' patch (available here) and this GPR being that in his patch, he already has enough information to "go on" even if repr doesn't return what one usually would expect.
That's why he could get by without fixing the actual issue, and the reason why it is less obvious that we can (as we don't have enough information and must then make conscious decisions about what to do when in an ill-typed context).

A better patch, which is now on my todo list (but wouldn't make to 4.06 even if I had the time to write it today) is to fix what I regard as the underlying issue.
Considering that, there are two options for this GPR:

  • merge it as is and revert the changes in a later patch (which will itself be in 4.07)
  • close this PR

But there's no point in delaying this PR for a later date.

I personally would go with option 1, as

  • I think it makes things slightly better in the short
  • I consider the risk to be minimal.

Of course having @maranget's input would be nice, but I think in this case the question really is: do we really expect anything from parmatch when the environment is inconsistent?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 24, 2017

If @garrigue had a clear recommendation ("I would like to merge this into 4.06", or "I would like to merge this into trunk only", or "let's wait"), we could consider following it, but right now I haven't perceived any of that from the thread (nor did @vprevosto indicate that they did a careful review of the patch and would be willing to bet to talk at the next OCaml Paris Meetup if an issue was uncovered in the future).

Also:

  • from the outside, the problem seems somewhat subtle
  • the problem that is fixed is the compiler failing with a non-ergonomic error instead of failing with an ergonomic error

I don't see myself thinking about it in my sleep if the issue only gets merged in trunk.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Oct 24, 2017

@gasche In that case, given what @trefis recommends (the two options), we should presumably close this. It's a shame for there to be wasted work though.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 24, 2017

I don't know how serious @trefis is when he says that merging the present PR in trunk is no better than closing it -- it does not ring very convincing to me.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Oct 24, 2017

I don't see any reason to believe that he was not serious. If the fix were to have been merged in 4.06, there would potentially have been some community benefit, whereas merging it into trunk and shortly afterwards replacing it with another patch (which should fix all of these issues properly) will probably not have that effect.

@vprevosto

This comment has been minimized.

Copy link
Contributor

vprevosto commented Oct 24, 2017

@mshinwell the potential benefit is only for users that enable warning 4 (fragile pattern matching). Given that this warning is extremely tedious to work with as soon as you have some moderately large variant types, I'd tend to think that it's a very tiny portion of the user base, although I don't have any number to back that claim. Again, this is also why there was no separate PR from #1071 on this specific topic from my side.

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Oct 25, 2017

Sorry if I appeared to be unclear.
It seems clear to me that this code is not dangerous: the only place where it is used is for warning 4.
I leave others to decide how important it is to fix this in 4.06.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 25, 2017

I leave others to decide how important it is to fix this in 4.06.

Well clearly it is not important at all to fix this in 4.06, except that @trefis and @mshinwell are trying to guilt-trip us into merging the patch on the day I originally marked in my calendar as "release".

Scrap that, I'm rebasing the patch against trunk.

@gasche gasche closed this Oct 25, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 25, 2017

Merged in trunk as 7b1140f.

@dra27 dra27 removed this from the 4.06.0 milestone Oct 25, 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.