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

Don't retypecheck erroneous arguments when fixing function #14043

Merged
merged 7 commits into from
Jan 24, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Dec 5, 2021

Don't retypecheck erroneous arguments when trying conversions or extensions
on the function part.

Generally, we should avoid typechecking untyped trees several times, this can quickly
explode exponentially.

Fixes #12941

@odersky
Copy link
Contributor Author

odersky commented Dec 6, 2021

This had to be refined considerably since we had failures in scaladoc, specs2 and spire. The new test checks whether
an argument failed to typecheck somewhere in its interior. Errors at the outside, under type ascription, or in closure parameters are ignored. If an argument fails to typecheck in its interior, we should not try to typecheck it again with a different function. Two reasons:

  1. This can lead to really bad slow downs if a failure is embedded in many nested large arguments, each of which would be checked exponentially often wrt to the nesting depth of arguments. protoquill hadf a case like this.

  2. This can leak unintended error messages if an argument is re-checked (and then fails again) with a different expected type. This is Wrong compiler error message while using contextual functions #12941.

@@ -75,7 +75,7 @@ class CommunityBuildTestC:
// @Test def oslibWatch = projects.oslibWatch.run()
@Test def playJson = projects.playJson.run()
@Test def pprint = projects.pprint.run()
@Test def protoquill = projects.protoquill.run()
// @Test def protoquill = projects.protoquill.run() // needs to fix new type inference errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling protoquill like this is risky: we have at least one open PR which benefits highly from being tested against it (#12540 /cc @nicolasstucki), how much work do you think it would take to fix the new errors?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a protoquill branch with workarounds for this change:

It seems that it is a problem with the insert function overloading. I will try to find a better fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is to change the API of protoquill to avoid the problematic insert overload as I id in zio/zio-protoquill@5ed25aa.

I suggest using dotty-staging/protoquill@1cc6154 for this PR as it is less intrusive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this, could you propose one of these changes to the author of protoquill?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deusaquilus this PR affects protoquill. Could you have a look at the discussion above?

Copy link
Contributor

@deusaquilus deusaquilus Dec 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no other choice I'll begin the API-change. Probably want this to be consistent with Scala2-Quill API first since I want them to be aligned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be a pester and ask again. Would it help the situation at all if lift was not a macro but a regular method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. @nicolasstucki do you have an idea?

Copy link
Contributor

@nicolasstucki nicolasstucki Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I remember, making lift a normal function might help.

But it feels like it should be fixed at the insert/update methods.

Copy link
Contributor

@nicolasstucki nicolasstucki Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also pushed the simple workaround (dotty-staging/protoquill@1cc6154) to this PR. We can change the code in protoquill now or after merging this PR. It might be easier to modify protoquill once we have a nightly build with this change.

@odersky
Copy link
Contributor Author

odersky commented Jan 24, 2022

protoquill still fails in the community build. How can we best fix this? I am anxious to get this PR in since we might get other failing CB tests otherwise. Note that PR restricts what can be inferred.

@odersky odersky removed their assignment Jan 24, 2022
@nicolasstucki
Copy link
Contributor

@odersky, I pushed the change fix zio/zio-protoquill@5ed25aa which is also in protoquill upstream zio/zio-quill#2379. The tests passed on my machine. This should not be a problem anymore because the ambiguous extension method signature was removed from protoquill.

@nicolasstucki
Copy link
Contributor

Protoquill passed. Something else failed, seems to be the issue from #14332

@odersky

This comment has been minimized.

@odersky
Copy link
Contributor Author

odersky commented Jan 24, 2022

@nicolasstucki Ah, I was looking at the state before your commit. Good that this is fixed.

@odersky
Copy link
Contributor Author

odersky commented Jan 24, 2022 via email

odersky and others added 7 commits January 24, 2022 15:22
Don't retypecheck erroneous arguments when trying conversions or extensions
on the function part.

Generally, we should avoid typechecking untyped trees several times, this can quickly
explode exponentially.

Fixes scala#12941
Only count errors that lead to an error type in some subtree as unrecoverable.
This should fix the failures in spire and specs2.
Protquill had to be disabled since it exhibited deeply nested errors in a function argument that went away
once an enclosing function was changed with a conversion or extension, and the full argument was completely typechecked.
Retype checking large bodies of code like this was never inteneded and counts as a bug in the compiler. Unfortunately,
this means that some type annotations will have to be added to protoquill to make it compiler again.
This reverts commit ea8d8b6.
Cherry-pick changes from zio/zio-protoquill@5ed25aa
also present in zio/zio-quill#2379.
@odersky
Copy link
Contributor Author

odersky commented Jan 24, 2022

@smarter Can you give this a quick review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong compiler error message while using contextual functions
6 participants