-
Notifications
You must be signed in to change notification settings - Fork 184
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 new configuration options to Disable rule #601
Conversation
"When true also blocks symbols in the generated code. Default to false. " + | ||
"Ignored if unlessInsideBlock is specified.") | ||
unlessSynthetic: Boolean, | ||
@Description("The unsafe that are banned unless inside a 'safe' block") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsafe symbols
| ] | ||
| } | ||
| { // unlessInsideBlock mode | ||
| unlessInsideBlock = "com.IO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this example? I think it's enough to have one example and scala.util.Try
seems like the most easy to understand
@@ -49,7 +59,7 @@ case object Disable { | |||
} | |||
val yy = AA.asInstanceOf // OK, no errors | |||
Option(1).get /* assert: Disable.Option.get | |||
^ | |||
^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? The caret should point to get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use ctx.index.symbol(tree)
to get symbol during tree traversal. Looking at sources for this method we have case Term.Select(_, name @ Name(_)) => symbol(name)
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should report only on the Name
, if you are in a pattern match the best way to do it is with case index.Symbol(tree: Name) => ...
symbols: List[CustomMessage[Symbol.Global]] = Nil | ||
@Description("The list of disable configurations") | ||
@ExampleValue("""[ | ||
| { // default mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should preserve the old syntax for Disable.symbols
, the new layout is not user-friendly
Disable.parts = [
{ symbols = [
...
]}
]
Instead of parts
, I would prefer
Disable.symbols = [...]
Disable.unlessSynthetic = [ ... ]
Disable.unlessInsideBlock = [ ... ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by how unlessSynthetic
works here. Is it a "disable this even if it is synthetic" setting? Neither symbols
nor unless
should operate on synthetic symbols.
@@ -5,10 +5,22 @@ title: Disable | |||
|
|||
# Disable | |||
|
|||
_Since 0.5.0_ | |||
_Since 0.6.0_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this unchanged and create a new "v0.6.0" section at the bottom that describes the new configuration options.
The website goes live as soon as this is merged so we should not immediately replace the v0.5 documentation, that would be confusing for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how can we keep the old version if we generate it automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stable version is currently hardcoded for 0.5.10 https://scalacenter.github.io/scalafix/docs/users/installation had to do that before releasing 0.6.0-M1 #610
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And how we can describe the new format then? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy paste the current generated markdown file and inline it. You'll need to generate the site first.
@@ -1,62 +0,0 @@ | |||
--- | |||
layout: docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of removing this file we should add a note saying this rule has been merged with Disable
and include migration instructions. People may have linked to this page so we should not return 404.
Any more comments on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a partial review yesterday that I forgot to submit. Took another round today and found more comments.
In the future, please try to use more descriptive commit messages @vovapolu. A large commit doing multiple things with a commit message "Fixes" makes it difficult to review. A list of high level bullet would help a lot, something like
Address review feedback
- Keep old DisableUnless.md
- Rename foo to bar
- Update description for setting X
|[ | ||
| { | ||
| symbol = "scala.Any.asInstanceOf" | ||
| message = "use patter-matching instead" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pattern
Right(config.allSymbols) // reset blocked symbols in def | ||
case (_: Term.Function, _) => | ||
Right(config.allSymbols) // reset blocked symbols in (...) => (...) | ||
case (t: Name, blockedSymbols) if treeIsBlocked(t, blockedSymbols) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case (ctx.index.Symbol(t: Name), ...
instead of val Some(...) =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymbolMatcher implements unapply(Tree)
, so this can even be simplified to something like
val badSymbol = SymbolMatcher.normalized(blockedSymbols:_*)
...
case (badSymbol(t: Name), ... =>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note, the scalafix API typically tries to make it possible to guard against stuff in the pattern instead of the guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olafurpg blockedSymbols
isn't static, it changes during tree traversal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can construct a new SymbolMatcher whenever it changes https://github.com/scalacenter/scalafix/blob/master/scalafix-core/shared/src/main/scala/scalafix/util/SymbolMatcher.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val Some(...) = ...
is a sign that you were not able to prove to the compiler this deconstruction can't fail. By using SymbolMatcher.unapply
you don't need to outsmart the typechecker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with val Some(...) = ...
.
Why can't you ignore cases when the tree isn't blocked with SymbolMatcher?
Well, in SearcherWithContext
I need to return new context or LintError
, ignoring means no changing in the context. I could just return an old context... I didn't want to do this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow why SymbolMatcher is not suitable here but I trust your judgment 😉 we can keep it unchanged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it to what you want, maybe it will be more understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, IDEA can't parse case (ctx.index.Symbol(t: Name), ...
@@ -5,6 +5,94 @@ title: Disable | |||
|
|||
# Disable | |||
|
|||
_Since 0.6.0_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the current docs at the top and the "Since v0.6.0" part at the bottom. I expect v0.6.0 to get released in a couple weeks from now so having this at the top would be confusing for users who visit the website today.
|
||
#### UnlessInsideBlock | ||
|
||
<table><thead><tr><th>Name</th><th>Type</th><th>Description</th></tr></thead><tbody><tr><td><code>unless</code></td><td>Symbol</td><td>The symbol that indicates a 'safe' block.</td></tr><tr><td><code>symbols</code></td><td>List[Message[Symbol]]</td><td>The unsafe symbols that are banned unless inside a 'safe' block</td></tr></tbody></table> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like configuration for the new rule. We should copy-paste the configuration for the old rule so keep the Disable site is unchanged for the "since v0.5.0" part
It should look like this
Config for the latest version of the rule is auto-generated with the scalafix.website.rule("Disable", DisableConfig.default)
line below
@@ -5,6 +5,10 @@ title: DisableUnless | |||
|
|||
# DisableUnless | |||
|
|||
_Since 0.6.0_ | |||
|
|||
This rule was merge into `Disable`. See `Disable` docs for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE. This rule is deprecated and from v0.6.0 onwards will be merged into the {% rule_ref Disable %} rule.
{ | ||
unless = "test.DisableUnless.IO" | ||
unless = "test.DisableUnlessInsideBlock.IO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this field be called "safeBlock" or "block" instead of "unless"?
Disable.unlessInsideBlock = [
{ safeBlock = "IO",
disabledSymbols = ... }
]
?
Any thoughts @fommil ? I personally haven't used this rule, but "unless = X, symbols = YY" seems weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the names of these things are starting to get quite big, but I think a lot of symbols will have a shared safeBlock
so it makes sense for them to be grouped like this. Each symbol may have a different explanation, though, so as long as that is supported we're good.
def filterBlockedSymbolsInBlock( | ||
blockedSymbols: List[Symbol.Global], | ||
block: Tree): List[Symbol.Global] = { | ||
val Some(symbolBlock: Symbol.Global) = ctx.index.symbol(block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can crash with MatchError, please refactor to avoid unsafe code.
/** | ||
* Searches for specific nodes in Tree and adds result of searching to the list. | ||
* Also passes context through tree traversal. | ||
* fn can either return a value. that stops the searching, or change the context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param fn If fn returns Left then XXX. If fn returns Right then YYY.
It's not immediately clear from the signature what fn
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this PR is just moving code around but given that Disable
is covering more use-cases after this PR we should try to document better what SearcherWithContext does.
* Also passes context through tree traversal. | ||
* fn can either return a value. that stops the searching, or change the context. | ||
*/ | ||
class SearcherWithContext[T, U](initContext: U)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContextTraverser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/T/Value/
s/U/Context/
?
object Disable { | ||
|
||
/** | ||
* Searches for specific nodes in Tree and adds result of searching to the list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tree traverser to collect values with a custom context.
At every tree node, either builds a new context
U
or returns a new valueT
to accumulate.
To collect all accumulated values, useresult(Tree)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D I'm not very experienced in writing docs, unfortunately.
* Changes in Disable tree traversal code * SearcherWithContext's renamed to ContextTraverser, the docs are updated * DisableConfig minor renamings * Disable and DisableUnless docs are also updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Disable.ifSynthetic deprecate NoInfer? If so, we should deprecate the NoInfer name and update it's docs to point to Disable in a specific "Since v0.6.0" section.
Only minor comments remaining, this is very close to complete!
object Test { | ||
val z = 1.asInstanceOf[String] // default mode, not ok | ||
|
||
List(1) + "any2stringadd" // unlessSynthetic mode, not ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/unlessSynthetic/ifSynthetic/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-executable docs are difficult to maintain, would be nice if we could somehow reuse testkit for docs 🤔
| } | ||
|] | ||
""".stripMargin) | ||
unlessInsideBlock: List[UnlessInsideBlock] = Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about renaming this to "unlessInsideSafeBlock"? I think it makes sense to be consistent betweeen .unlessInsideBlock
and safeBlock =
. Standalone "Block" is too generic so I think "safeBlock" may be clearer, but I'll leave it to you and @fommil to decide what makes most sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the concept of "safe"ness is perhaps limited to my usecase. I can imagine people wanting to use this for other purposes, such as logging or monitoring, in which case "safe" will be a misnomer. The "block" concept is also a bit of a misnomer because of course it might not be in a block at all but just a regular method call... IO.apply(println("hello world"))
has no blocks. So 👍 for unlessInside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. What should the "safe symbol" be called then?
Disable.unlessInside = [ ??? ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any nuke setting to disable a symbol regardless if it's synthetic or not?
Also, should we deprecate NoInfer in favor of Disable.ifSynthetic?
| "com.Lib.implicitConversion" | ||
|]""".stripMargin) | ||
symbols: List[CustomMessage[Symbol.Global]] = Nil | ||
@Description("The list of symbols to disable only in the actual sources.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of symbols to disable if written explicitly in source. Does not report an error for inferred symbols in macro expanded code or implicits.
|]""".stripMargin) | ||
symbols: List[CustomMessage[Symbol.Global]] = Nil, | ||
@Description( | ||
"The list of symbols to disable, also blocks symbols in the generated code") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of symbols to disable if inferred. Does not report an error for symbols written explicitly in source code.
Add it to the
Why not? :) |
can we please do any more refinements in a follow up? I'm really keen to try this out in the wild once you cut a release and I fear we will run out of Vladimir's sponsored time by the time I discover anything. |
That seems a bit tedious for a seemingly common use-case. Would be good to have an option to enable both synthetic and non-synthetic checks and
Sure, we can do that esp. since we are kind of in between major releases. Keep in mind that the current master is a bit in a limbo in a transition to the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Great refactoring @vovapolu !
I left nitpick comments which I'd like to see addressed in a followup PR. I would also like to see Disable.symbols
trigger both synthetic and explicit symbols and Disable.ifExplicit
replace the current Disable.symbols
.
|
||
It has several different modes: | ||
- Default mode. In this mode symbols are banned in the actual source code. | ||
- `unlessInsideBlock` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unlessInside
|
||
List(1) + "any2stringadd" // ifSynthetic mode, not ok | ||
|
||
Option.empty[Int].get // unlessInsideBlock mode, not ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unlessInside
Initial issue #598.
This PR adds new configuration options
Disable.ifSynthetic
: to disable symbols in expanded implicits and macro codeDisable.unlessInside
: superseding the oldDisableUnless rule.
Additionally, this PR refines
Disable
to discard non-significant references to banned symbols such as imports.