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

Add FinalVal wart #237

Merged
merged 3 commits into from
Jul 12, 2016
Merged

Add FinalVal wart #237

merged 3 commits into from
Jul 12, 2016

Conversation

tim-zh
Copy link
Contributor

@tim-zh tim-zh commented Jun 15, 2016

No description provided.

@tim-zh
Copy link
Contributor Author

tim-zh commented Jun 28, 2016

any update on this?

should i add the following example to readme?

$ echo "object a { final val v = 1 }" > a.scala
$ echo "object b extends App { println(a.v) }" > b.scala
$ sbt ~run
1
$ echo "object a { final val v = 2 }" > a.scala
1

@ClaireNeveu
Copy link
Collaborator

What is the proposed workaround for this situation?

@ClaireNeveu
Copy link
Collaborator

Also, is there a bug filed against scala that we can link to?

@dwijnand
Copy link
Collaborator

It's in the spec, it's expected behaviour.

@tim-zh
Copy link
Contributor Author

tim-zh commented Jun 28, 2016

workaround is to use non-final vals

it's not a Scala bug, but it's still quite unsafe

@ClaireNeveu
Copy link
Collaborator

@dwijnand Inlining is certainly in the spec but the incremental compiler not recompiling dependent files due to inlining is not (to my knowledge).

@tim-zh My concern there is that making a val non-final changes the semantics and people use final for a reason. I think we could say in the error message to use final def instead which does not seem to break incremental compiliation but preserves the desired semantics.

@dwijnand
Copy link
Collaborator

Yeah, I agree with you there. We should open a ticket against Zinc to fix it.

@dwijnand
Copy link
Collaborator

dwijnand commented Jul 6, 2016

Btw it's constant-folding, not inlining.

And making it a def or adding a type ascription to the val are other alternatives.

val v: Int = 1
def v: Int = 1
final val v: Int = 1

@tim-zh
Copy link
Contributor Author

tim-zh commented Jul 6, 2016

@dwijnand Indeed, thanks. It's a bit trickier than i thought, and it means the pr can't be merged :(, since we can't prevent false positives right now.

@ClaireNeveu
Copy link
Collaborator

@tim-zh I think the only false-positive you're missing is final val v: Int = 1, which doesn't get folded. You can take the approach from ExplicitImplicitTypes to check for the annotation. Unlike with defs, the annotation should always be on the same line for vals. The only false positives we should see there are final vals generated by macro expansion which we'll hopefully have an answer for soon.

@ClaireNeveu ClaireNeveu added this to the 1.1.0 milestone Jul 7, 2016
@ClaireNeveu
Copy link
Collaborator

This looks set to merge to me and it'll go out in 1.1.0. The one additional thing I'd like to see is a ticket for the bug against Zinc so we can link to it in the README. Since this wart is related to an actual bug I want to know when it stops being relevant (due to a bugfix).

@tim-zh
Copy link
Contributor Author

tim-zh commented Jul 12, 2016

@ChrisNeveu sbt/sbt/issues/1543 ?

@ClaireNeveu
Copy link
Collaborator

Thanks! I'll add a link to that in the README.

@ClaireNeveu ClaireNeveu merged commit 04ed570 into wartremover:master Jul 12, 2016
@adriaanm
Copy link

adriaanm commented Mar 9, 2023

by the way, the incremental compiler now tracks these kind of constant vals

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

Successfully merging this pull request may close these issues.

4 participants