Record failures to adapt application arguments

[dwijnand]

In tests/pos/i18163.scala, we don't want the ambiguous implicit of Inv[J] to cause the selection of LogOps to fail. The logic is there because it think the user's implicits "seem to be used to implement a local failure in order to negate an implicit search". But that's not the case here. The regular arguments aren't correct to start with (type kindness error), so we shouldn't even get to the implicit search and ambiguity.

[dwijnand]

@odersky what do you think of this fix? This is accidentally triggering the "seem to be used to implement a local failure in order to negate an implicit search" logic. I'm not sure how to fix that, so I went with this. If you think it's good, I'll fix up the remaining tests.

[odersky]

I am not sure. I think in general it's better o leave type inference alone unless there's a very strong reason why it should be changed. The errors in the tests don't look like an improvement, rather the opposite. I.e. more "expected: Nothing" than before.

[dwijnand]

I'll study the misfiring error regarding "NotGiven".

[dwijnand]

@odersky how do you feel about dropping the misfiring error? Or can you spot or think of why it's misfiring?

[Kordyjan]

According to the full run of the Open Community Build, this PR is not causing any new failures. Compare this with that.

[dwijnand]

Either something went askew, and the fix isn't actually being run against valskalla/odin, or the minimisation went too far? Can I ask @WojciechMazur to have a little look?

[odersky]

The code to evaluate or not arguments is very tricky and has lots of pitfalls. I don't think the benefits gained from the fix are worth the risk of changing the code.

[WojciechMazur]

@dwijnand I looks like we had some kind of bug when running two OCB in short time span. In fact the build using https://github.com/dotty-staging/dotty/tree/pr18239_pr18286_pr18269 was skipped. It's due to our logic, which skips subsequent builds with the same Scala version. It's done by checking our custom maven repository if artifacts of given project were published (we should change this logic to check if any of tasks has failed) for given version of compiler. It seems like for some reason the SHA of compiler version was incorrectly calculated leading and was the same as for the build of https://github.com/dotty-staging/dotty/tree/pr18239_pr18286
I would fix these issues and re run the Open CB this evening

[Kordyjan]

I think this is only a problem with generated reports. If we go to the builds, we can see that valskalla/odin is green in the second one, but red in the first one.

So everything seems to be in order!

[dwijnand]

valskalla/odin is green in the second one, but red in the first one.

Oh even better, so this is does fix the community project.

[odersky]

This looks still wrong to me. Turning an ambiguous implicit to a non matching implicit simply does not make sense. If the tests are green it just means we are lacking the right tests.

This was one of the biggest shortcomings of Scala 2's ad-hoc approach to implicits. We won't repeat it in Scala 3.

[dwijnand]

I was only reverting to the preexisting logic. I understand now how it was more Scala 2 specific than I had understood. All tests pass without converting the failure (at least locally).

[WojciechMazur]

The valskalla/odin passes Open CB build when using https://github.com/dotty-staging/dotty/tree/pr18239_pr18286_pr18269
Logs: https://github.com/VirtusLab/community-build3/actions/runs/5751695588/job/15599631091

[Kordyjan]

According to the build report from the rerun, there are two new regressions. Both of them are caused by problems with the implicit search:

• hnaderi/lepus
• neandertech/langoustine

[odersky]

The explain a bit more:

An ambiguous implicit is different from a no-matching implicit in that it is sticky. The only way to heal an ambiguity between A and B is to find a C which is better than both. But if A/B ambiguous was mapped to NoMatchingImplicits and the search proceeds with a matching C then C will be picked even if it is not better than A or B. Also, in that case implicit search order would determine the outcome. All things we definitely do not want. It was a mistake in Scala 2, that unfortunately got exploited by the community to implement negation.

[dwijnand]

Thank you all. I'll shift the fix again.

[dwijnand]

So I've gone back to my original way of fixing this, which was to deal with application arguments failing to adapt to their expected type. I had originally moved away from that due to this comment:

The errors in the tests don't look like an improvement, rather the opposite. I.e. more "expected: Nothing" than before.

I had fallen into the same misunderstanding, but it turns out that in tests/neg/enum-values.scala there is an explicit Nothing expected type, so it's not as bad as it looked.

Locally I think the test expectations look ok, but I want to double check in CI.

[Kordyjan]

OCB run for the latest changes: https://github.com/VirtusLab/community-build3/actions/runs/5784172808.

[odersky]

What issue are we trying to fix here? Is it the regression caused by #16786? In fact going back to that I now believe that that change does not make sense. I think it's best to revert #16786 rather than to perturb type inference even more.

Some aspects of type inference are ad-hoc choices. Changing these choices can solve some problems but can also create new ones. I believe we are now at a stage where we should stop making these changes, unless we can convince ourselves by looking at the code that they will not introduce new errors.

[dwijnand]

What issue are we trying to fix here? Is it the regression caused by #16786? In fact going back to that I now believe that that change does not make sense. I think it's best to revert #16786 rather than to perturb type inference even more.

I don't understand why you think it doesn't make sense. It seems to be in line with how things are done, it was just a missing case, that was causing an inconsistency in what I consider to be a valid issue: #14218. And the fix I'm proposing here makes sense to me: making TypedApply consider the result of addArg in its success status, consistent with TestApplication, which does so in its addArg.

It's unfortunate that fixing one bug reveals another, but I don't think we should freeze everything completely.

[odersky]

I left a comment on #16786 with my concerns.

[dwijnand]

Rebasing over #18352.

[Kordyjan]

Combined PRs #18239, #18286, #18352 & #18269 don't cause new regressions.

So, once this PR is merged I will go forth with the RC5 release.

[odersky]

The changes look good to me.

[dwijnand]

valskalla/odin is now in red again, but I assume it is the expected outcome now.

Actually it's green!

[tgodzik]

Actually, why the change to the test here? Could you possibly ping me or anyone from the Metals team next time something within the presentation compiler needs to change? That would be greatly appreciated 🙏

[dwijnand]

Sure. It's because otherwise double doesn't typecheck, because String * Any doesn't typecheck.

[tgodzik]

Ach, right! Makes sense!