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 safe-init test to prevent adding cold elements to hot arrays #14895

Merged
merged 2 commits into from
May 9, 2022

Conversation

Xavientois
Copy link
Contributor

This adds a negative test to ensure that the initialization checker doesn't allow cold values to be inserted into hot arrays through a method call.

@Xavientois
Copy link
Contributor Author

Test currently fails due to case not being handled correctly

@griggt
Copy link
Contributor

griggt commented Apr 8, 2022

@Xavientois FYI opening PR branches directly in the lampepfl/dotty repository causes CI to run twice for every push:

https://github.com/lampepfl/dotty/actions/runs/2116548617
https://github.com/lampepfl/dotty/actions/runs/2116555314

which is one of the reasons we don't normally do that.

@olhotak
Copy link
Contributor

olhotak commented Apr 8, 2022

The usual thing to do is to push a branch to https://github.com/dotty-staging/dotty (or a fork in your own account).

def foo[T](x: T, array: Array[T]): Unit = array(0) = x
var a = Array.apply(1, 2, 3)
foo(i, a)
val i = 99
Copy link
Contributor

Choose a reason for hiding this comment

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

Here i is not cold. You can use e.g. this instead. It's better to only leave the method foo in the global object A, and move other code to a class --- this way, we can be sure that the method call foo(...) will be A.foo(...) instead of A.this.foo(...).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not clear enough in the above. I think something like the following will serve the purpose:

class B {
  var a = new Array[B](2)
  A.foo(this, a)
  println(a(0).i)
  val i = 99
}

@Xavientois
Copy link
Contributor Author

@Xavientois FYI opening PR branches directly in the lampepfl/dotty repository causes CI to run twice for every push:

Sorry about that! I'll be sure to open PRs from my fork in the future

@Xavientois Xavientois force-pushed the cold-insert-hot-array-test branch from 9eaa220 to 2ccbe8c Compare April 11, 2022 15:20
@Xavientois Xavientois requested a review from liufengyun April 11, 2022 15:20
val i = 99
}

val b = new B()
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this line.

@Xavientois Xavientois force-pushed the cold-insert-hot-array-test branch 2 times, most recently from 8a1adb2 to 28ca899 Compare April 13, 2022 14:10

class B {
var a = new Array[B](2)
A.foo(this, a) //error
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a space before error

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

@som-snytt
Copy link
Contributor

For the record, I think that with all the interesting dimensions of freedom, the precise formatting of // error ought not to be a hangup. IIRC I pushed back on // scalac: in test files. My point is that testing is already hard, and any cost in making it easier is amortized over the life of the project.

Xavientois and others added 2 commits May 9, 2022 10:57
This adds a negative test to ensure that the initialization checker does
nt allow cold values to be inserted into hot arrays through a method
call.
@olhotak olhotak force-pushed the cold-insert-hot-array-test branch from 132b278 to 7c26c7e Compare May 9, 2022 08:59
@olhotak olhotak merged commit cf3b59a into main May 9, 2022
@olhotak olhotak deleted the cold-insert-hot-array-test branch May 9, 2022 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants