Don't retypecheck erroneous arguments when fixing function #14043

odersky · 2021-12-05T13:20:34Z

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.

odersky · 2021-12-06T11:24:47Z

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:

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.
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.

tests/pos/zipped.scala

smarter · 2021-12-13T12:53:19Z

community-build/test/scala/dotty/communitybuild/CommunityBuildTest.scala

@@ -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


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?

I created a protoquill branch with workarounds for this change:

dotty-staging/protoquill@1cc6154

https://github.com/dotty-staging/protoquill/tree/workaroud-typing-changes-in-dotty-%2314043

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

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.

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

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

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.

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?

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

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

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

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 · 2022-01-24T10:08:23Z

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.

nicolasstucki · 2022-01-24T10:31:15Z

@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 · 2022-01-24T12:02:30Z

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

odersky · 2022-01-24T12:42:01Z

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

odersky · 2022-01-24T12:57:04Z

I fixed the issue from #14332 here.

…

On Mon, Jan 24, 2022 at 1:02 PM Nicolas Stucki ***@***.***> wrote: Protoquill passed. Something else failed, seems to be the issue from #14332 <#14332> — Reply to this email directly, view it on GitHub <#14043 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVTHH7EMGPJ2LUBB663UXU5WNANCNFSM5JMY4GXQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Martin Odersky Professor, Programming Methods Group (LAMP) Faculty IC, EPFL Station 14, Lausanne, Switzerland

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 · 2022-01-24T15:03:32Z

@smarter Can you give this a quick review?

odersky commented Dec 6, 2021

View reviewed changes

tests/pos/zipped.scala Show resolved Hide resolved

mhzajac mentioned this pull request Dec 10, 2021

Wrong compiler error message while using contextual functions #12941

Closed

odersky requested a review from smarter December 13, 2021 10:10

odersky assigned smarter Dec 13, 2021

smarter reviewed Dec 13, 2021

View reviewed changes

nicolasstucki assigned odersky Dec 16, 2021

odersky force-pushed the fix-12941 branch 2 times, most recently from 3f4347a to cc5428f Compare December 19, 2021 12:13

odersky force-pushed the fix-12941 branch from 5ec4e09 to f352f00 Compare January 3, 2022 17:19

deusaquilus mentioned this pull request Jan 13, 2022

Need to change EntityQuery.insert(CaseClass) to EntityQuery.insertValue(CaseClass) for upstream Scala 3 issues. zio/zio-quill#2379

Merged

odersky force-pushed the fix-12941 branch from f352f00 to 8b08ad1 Compare January 23, 2022 17:21

odersky removed their assignment Jan 24, 2022

This comment has been minimized.

Sign in to view

odersky and others added 7 commits January 24, 2022 15:22

Don't retypecheck erroneous arguments when fixing function

002f9a0

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

Refine condition when to not typecheck again

a3e2840

Only count errors that lead to an error type in some subtree as unrecoverable.

Further refine condition when not to typecheck again

fcf2560

This should fix the failures in spire and specs2.

Harden hasInnerErrors and fix tests

4d0d205

Revert "Disable protoquill"

ee015bc

This reverts commit ea8d8b6.

Fix protoquill

89ec460

Cherry-pick changes from zio/zio-protoquill@5ed25aa also present in zio/zio-quill#2379.

odersky force-pushed the fix-12941 branch from baf8f9d to 89ec460 Compare January 24, 2022 14:23

dwijnand approved these changes Jan 24, 2022

View reviewed changes

smarter approved these changes Jan 24, 2022

View reviewed changes

nicolasstucki enabled auto-merge January 24, 2022 16:11

nicolasstucki merged commit 0e4e5d5 into scala:master Jan 24, 2022

nicolasstucki deleted the fix-12941 branch January 24, 2022 16:27

This was referenced Jan 28, 2022

Remove deprecated EntityQuery.update/insert APIs zio/zio-quill#2412

Merged

Change update to updateValue zio/zio-quill#2405

Merged

Kordyjan added this to the 3.1.2 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't retypecheck erroneous arguments when fixing function #14043

Don't retypecheck erroneous arguments when fixing function #14043

odersky commented Dec 5, 2021

odersky commented Dec 6, 2021 •

edited

Loading

smarter Dec 13, 2021

nicolasstucki Dec 16, 2021

nicolasstucki Dec 16, 2021

smarter Dec 16, 2021

nicolasstucki Dec 16, 2021

deusaquilus Dec 19, 2021 •

edited

Loading

deusaquilus Dec 28, 2021

odersky Dec 31, 2021

nicolasstucki Jan 3, 2022 •

edited

Loading

nicolasstucki Jan 3, 2022 •

edited

Loading

odersky commented Jan 24, 2022

nicolasstucki commented Jan 24, 2022

nicolasstucki commented Jan 24, 2022

This comment has been minimized.

odersky commented Jan 24, 2022

odersky commented Jan 24, 2022 via email

odersky commented Jan 24, 2022

Don't retypecheck erroneous arguments when fixing function #14043

Don't retypecheck erroneous arguments when fixing function #14043

Conversation

odersky commented Dec 5, 2021

odersky commented Dec 6, 2021 • edited Loading

smarter Dec 13, 2021

Choose a reason for hiding this comment

nicolasstucki Dec 16, 2021

Choose a reason for hiding this comment

nicolasstucki Dec 16, 2021

Choose a reason for hiding this comment

smarter Dec 16, 2021

Choose a reason for hiding this comment

nicolasstucki Dec 16, 2021

Choose a reason for hiding this comment

deusaquilus Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

deusaquilus Dec 28, 2021

Choose a reason for hiding this comment

odersky Dec 31, 2021

Choose a reason for hiding this comment

nicolasstucki Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

nicolasstucki Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

odersky commented Jan 24, 2022

nicolasstucki commented Jan 24, 2022

nicolasstucki commented Jan 24, 2022

This comment has been minimized.

odersky commented Jan 24, 2022

odersky commented Jan 24, 2022 via email

odersky commented Jan 24, 2022

odersky commented Dec 6, 2021 •

edited

Loading

deusaquilus Dec 19, 2021 •

edited

Loading

nicolasstucki Jan 3, 2022 •

edited

Loading

nicolasstucki Jan 3, 2022 •

edited

Loading