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

Prevent nesting properties #677

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

NthPortal
Copy link
Contributor

Prevent nesting properties or otherwise adding properties
during execution.

@NthPortal NthPortal force-pushed the topic/no-nested-properties/PR branch from eb0c945 to 92c74c3 Compare August 21, 2020 02:05
@@ -46,7 +47,10 @@ class Properties(val name: String) {

/** Returns all properties of this collection in a list of name/property
* pairs. */
def properties: collection.Seq[(String,Prop)] = props
def properties: collection.Seq[(String,Prop)] = {
frozen = true // once the properties have been exposed, they must be frozen
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an alternative would be to add a private[scalacheck] def freeze(): Unit and call it from the task when it gets the properties or right before execution

@SethTisue
Copy link
Member

elsewhere you said "the scalacheck folks said this is not something you're supposed to do", was on that on Twitter?

@NthPortal
Copy link
Contributor Author

@NthPortal
Copy link
Contributor Author

for reference, what currently happens when you nest properties is not particularly sensible in the first place. they do run, but their name/path does not include the property they were nested inside of, and they get executed at the end, rather than in order.

@SethTisue
Copy link
Member

does anyone object to this? if not, let's merge it (in a week or so, say)

@NthPortal
Copy link
Contributor Author

I don't see any objections

@SethTisue
Copy link
Member

okay, if you squash it, I'll merge it

Prevent nesting properties or otherwise adding properties
during execution.
@NthPortal
Copy link
Contributor Author

squashed

@SethTisue SethTisue merged commit f68aa77 into typelevel:master Sep 15, 2020
@NthPortal NthPortal deleted the topic/no-nested-properties/PR branch September 15, 2020 20:50
@ashawley ashawley added this to the 1.15.0 milestone Sep 22, 2020
@ashawley
Copy link
Contributor

What was the motivation for this change? I recall discussing in Gitter that there was an effort to optimize the Scala collections library that revealed a defect in this code in ScalaCheck. Is there an upstream issue to read more about this?

@NthPortal
Copy link
Contributor Author

There were two motivations for this change:

  • nesting properties was never supported, and it is better to explicitly disallow things you don't support rather than have them partially or mostly work and then have behaviour accidentally change underneath people if you end up changing the internal implementation
  • what happens if you nest properties is that it creates a new property while executing the rest of the properties, and adds it to the ListBuffer that's being iterated over in order to execute all of the properties. as of 2.13.4, it is very likely that ListBuffer iterators (and soon other types as well) will throw an exception if you modify the underlying collection while using them

@ashawley
Copy link
Contributor

Thanks, for the explanation. Had you run across a test suite that had nested properties and an exception was thrown with the ListBuffer change?

@NthPortal
Copy link
Contributor Author

yes. scala/scala's 😬

@ashawley
Copy link
Contributor

Ok, I wonder if I was wrong about nested properties not being supported. I know nesting properties isn't documented, but I suppose it may be possible for people to use it. I recall that cats and discipline projects could be doing something like this with law checking.

@SethTisue
Copy link
Member

@ashawley would you like me to run this through the Scala community build?

@ashawley
Copy link
Contributor

Yes, that would be useful. I was planning to ask. I'll open an issue there.

@SethTisue
Copy link
Member

We're good over at scala/community-build#1226, so it looks like this change will stand.

@ashawley
Copy link
Contributor

ashawley commented Oct 2, 2020

Yes, thanks for doing that for us, Seth!

SethTisue added a commit to scala-steward/scala-3 that referenced this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants