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

SI-7629 Deprecate view bounds #2909

Merged
merged 1 commit into from Sep 27, 2013
Merged

Conversation

@soc
Copy link
Member

@soc soc commented Sep 4, 2013

This introduces a warning(/error with -Xfuture) with a general
migration advice. The IDE can use the warning to offer a quick fix
with the specific refactoring necessary.

@soc
Copy link
Member Author

@soc soc commented Sep 4, 2013

Todo:

@xeno-by
Copy link
Member

@xeno-by xeno-by commented Sep 5, 2013

I think this commit would use a non-empty description.

@@ -2257,6 +2257,11 @@ self =>
}
if (contextBoundBuf ne null) {
while (in.token == VIEWBOUND) {
val msg = "Use an implicit parameter instead.\nE. g. `def f[A <% B](a: A)` --> `def f(a: A)(implicit ev: A => B)`."
Copy link
Member

@xeno-by xeno-by Sep 5, 2013

def f[A, B](a: A)(implicit ev: A => B)?

Copy link
Member Author

@soc soc Sep 5, 2013

Mhh, good question.

Then it would have to look like f[B, A <% B](a: A) --> f: [B, A](a: A)(implicit ev: A => B).
Maybe one should pick an actual existing bound as an example instead ...

@soc
Copy link
Member Author

@soc soc commented Sep 8, 2013

@xeno-by Updated.

@xeno-by
Copy link
Member

@xeno-by xeno-by commented Sep 9, 2013

@soc Looks like the proposed fix in the error message still lacks a type parameter :)

if (settings.future)
syntaxError(in.offset, "View bounds have been removed." + msg)
else
deprecationWarning(in.offset, "View bounds are deprecated." + msg)
Copy link
Member

@retronym retronym Sep 9, 2013

The message is missing whitespace. This should be tested in neg test with -Xfatal-warnings.

IntelliJ still incorrectly flags mixing implicit parameter lists and implicit bounds, we should get that fixed before we start instructing people to do this. http://youtrack.jetbrains.com/issue/SCL-3487

image

@soc
Copy link
Member Author

@soc soc commented Sep 9, 2013

Weird ... I wonder whether I messed something up. The tests are gone completely. I'll redo this commit again. Sorry guys!

@soc
Copy link
Member Author

@soc soc commented Sep 10, 2013

Ok, extremely weird. I can't get the tests to fail in partest, the compiler doesn't emit the warnings. Did anything change recently in regard to partest?

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 10, 2013

Not sure what you mean by "doesn't emit the warnings." I tried your branch and it WFM.

I'll try to PR to the PR, but if I fail, I suggest this version of the warning or similar:

        if (contextBoundBuf ne null) {
          while (in.token == VIEWBOUND) {
            contextBoundBuf += atPos(in.skipToken()) {
              val t = typ()
              def discourage() = {
                val msg = "Use an implicit parameter instead."
                val eg = s"Instead of `def $owner[$pname <% $t](p: $pname)`, use `def $owner(p: $pname)(implicit ev: $pname => $t)`."
                if (settings.future) syntaxError(in.offset, f"View bounds have been removed. $msg%n$eg")
                else deprecationWarning(in.offset, f"View bounds are deprecated. $msg%n$eg")
              }
              discourage()
              makeFunctionTypeTree(List(Ident(pname)), t)
            }
          }

where the change is to use the bounds in the example and use platform newline.

I will also suggest .flags with -deprecation with updated checks, or fix the test, or if testing a deprecated feature, deprecate the Main to suppress the warning. I forgot to check if view bound is being tested by these tests.

@soc
Copy link
Member Author

@soc soc commented Sep 10, 2013

Thanks @som-snytt. I have added tests locally to test/files/neg, but the issue is that they don't fail as expected. I'm starting to wonder whether partest somehow decided to pick the last stable Scala build instead of the one built from the source.

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 10, 2013

Of course, by fail as expected, you mean succeed by failing (as expected). If these two tests emit deprecations, then any neg test will, too, right? I'll try it, since it's set up.

Meanwhile, I'm not sure about the sample value param list; I considered an ellipsis instead:

iterator-from.scala:14: warning: View bounds are deprecated. Use an implicit parameter instead.
Instead of `def testSet[A <% Ordered[A]](p: A)`, use `def testSet(p: A)(implicit ev: A => Ordered[A])`.
  def testSet[A <% Ordered[A]](set: SortedSet[A], list: List[A]) {

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 10, 2013

Yes, to confirm, just copying over iterator-from to neg with -Xfatal-warnings and -deprecation will fail (i.e. correctly).

I did notice it has an inline warning filter; remember warnings are only emitted once for a location, so a test could theoretically emit inline warning (for instance), have it filtered by partest, but then because you were really interested in an another warning you expected at the same position, you wind up confused.

I don't know yet if that happened here, but it's a big caveat.

@retronym
Copy link
Member

@retronym retronym commented Sep 11, 2013

To run partest from the command line as it will be done in the test suite:

SCALAC_OPTS=-optimize ./test/partest path/to/test

If the test fails spuriously under -optimize, you can use a .flags files with -Ynooptimise.

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 11, 2013

Isn't that cheating? #partest -optimise might work in any case.

If you lose patience with it, just add #partest true to the checkfile and filter: .* to the test header. Something like that should work well enough so you won't get caught cheating, which is what they throw you in the slammer for, i.e., getting caught.

@retronym
Copy link
Member

@retronym retronym commented Sep 11, 2013

I'm not sure whether / how -optimize is related to this PR. In general, there are occasional good reasons to limit a test to -Ynooptimise, e.g. fine grained checks on trees or bytecode that would vary otherwise in ways not pertinent to what's being tested.

@soc
Copy link
Member Author

@soc soc commented Sep 11, 2013

Ok, found my stupid mistake (if I want to test the deprecation of <% I shouldn't use <: in the tests ...).

@som-snytt I adopted some of your wording because it is better.
I refrained from trying to build specific migration messages for now, because imho it is very hard to get a robust solution that early in the parser and I think it's better to have something clearly marked as a general example instead of trying to use the code in question and offering an imprecise and probably incorrect advice.

I think if you managed to get something working which would be robust for all inputs, e. g. def g[C, B <: C, A <% B : Numeric](a: A) = null this would be absolutely fantastic because then the IDE could take it's refactoring advice straight from the compiler.

@soc
Copy link
Member Author

@soc soc commented Sep 15, 2013

Review by @gkossakowski, please.

@gkossakowski
Copy link
Member

@gkossakowski gkossakowski commented Sep 22, 2013

@soc: I'm moving to Europe next week so I won't have time to look into this. Please assign other reviewer. @retronym, could you have a look?

@soc
Copy link
Member Author

@soc soc commented Sep 23, 2013

@gkossakowski Thanks for the notice! @retronym, could you have a look?

@soc
Copy link
Member Author

@soc soc commented Sep 23, 2013

PLS REBUILD/pr-scala@801866c

@scala-jenkins
Copy link

@scala-jenkins scala-jenkins commented Sep 23, 2013

(kitty-note-to-self: ignore 24949656)
🐱 Roger! Rebuilding pr-scala for 801866ce. 🚨

@retronym
Copy link
Member

@retronym retronym commented Sep 25, 2013

I'm still in two minds about merging this without giving a more specific instruction about how to fix it.

One task that is definitely still needed is a commit that migrates usages of view bounds in our codebase to implicit parameters.

@soc
Copy link
Member Author

@soc soc commented Sep 25, 2013

Yep. This is exactly what this commit is about. There already exists IDE code for both a quick fix and a refactoring. That code depends on getting a warning from the compiler to trigger properly.

This is what this commit enables:

  • Allow easier testing of the migration code/advice
  • Make automated actions solid
  • Move the appropriate parts of the then well-tested migration code into the compiler, so that all tools benefit from it

@retronym
Copy link
Member

@retronym retronym commented Sep 25, 2013

How about you in the first step you make it deprecated with -Xfuture and do nothing otherwise. Then, when the migration tools are shown to work, we can take the next step, hopefully before 2.11.0.

This introduces a warning(/error with -Xfuture) with a general
migration advice. The IDE can use the warning to offer a quick fix
with the specific refactoring necessary.
contextBoundBuf += atPos(in.skipToken()) {
makeFunctionTypeTree(List(Ident(pname)), typ())
}
val msg = "Use an implicit parameter instead.\nExample: Instead of `def f[A <% Int](a: A)` use `def f[A](a: A)(implicit ev: A => Int)`."
Copy link
Contributor

@adriaanm adriaanm Sep 26, 2013

"Use a context bound or an implicit parameter instead." --> wish we could link directly to an html-version of the spec here

@retronym
Copy link
Member

@retronym retronym commented Sep 27, 2013

I'm happy for this to go in under -Xfuture. Let us know how the migration tool works out and we can consider the next step.

retronym added a commit that referenced this issue Sep 27, 2013
@retronym retronym merged commit 5a8cd09 into scala:master Sep 27, 2013
1 check passed
raboof added a commit to raboof/scala.github.com that referenced this issue Oct 1, 2016
As they're deprecated (scala/scala#2909).
Most of that section describes the common alternative to view bounds,
though, so that probably deserves to remain in the FAQ
eed3si9n added a commit to eed3si9n/scala that referenced this issue Apr 8, 2018
Fixes scala/bug#10719

scala#2909 deprecated view bounds under -Xfuture. This deprecates it without it, and drops it under -Xsource:2.14.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants