ImportControl: global blacklist

[rnveach]

This may just be an issue with Checkstyle's own import control for now, but I noticed this when reviewing the import control file.

For all packages, we have a list of banned imports of java.io.File*Stream, java.util.stream.*, and java.util.Comparator. However, the gui sub-package with it's own isolated list of imports, allows java.io and allows java.util.* (with no sub-subpackages).

This means gui allows some imports that are banned on a global level. We have a global ban because these imports cause issues with some tools or there are better ways to code that may not be obvious. Because sub-packages are examined first, the only current way to correct this is to ban imports in 2 places, duplicating the list and reasoning.

Should we consider to create a new area that will be a global blacklist to ban imports everywhere and ignore other rules? Or maybe split the import control into 2, a global blacklist and a package by package whitelist?

[romani]

I think it is problem/nuance of our config only.

We have <subpackage name="gui" strategyOnMismatch="disallowed"> if we change strategyOnMismatch to delegateToParent, it will work like you want. This particular import ban planned to be addressed at #5616 .

[rnveach]

We have if we change strategyOnMismatch to delegateToParent, it will work like you want.

This is incorrect. Subpackage has a specific allow rule in it. strategyOnMismatch doesn't play a role in this as that rule comes into effect first. Even if it did, the strategy is a disallow which would mean the import is blacklisted no matter what. This issue is talking about a global blacklist because a whitelist is overriding it.

This particular import ban planned to be addressed at #5616 .

That only covers the cobertura ban, not the other one, so this is still an issue even if that one is fixed.

[romani]

you are right.

Here is typical configuration problem, user is responsible to keep it reasonable. This nuance is by design :). It is probably a good example of problems when both models (allow and disallow) are used. I am more in favor of blacklist model. White list model also make sense some time. But mix of models might be problematic to support by user.

We can convert our import control file to blacklist model for all except for API. only in API (the only area where we want to control all imports explicitly) use while-list model. Does it make sense ?

[rnveach]

We can convert our import control file to

If you want to do something like https://github.com/checkstyle/checkstyle/blob/master/src/xdocs/config_imports.xml#L1174-L1181 this is fine with me.

The only problem is making sure the disallowed list is complete and maybe this is why I prefer whitelist mode. With blacklist, you have to manually verify all imports are correct in new files and that no stray ones are accidentally being allowed in because you forgot a disallow. I say manually because there will be no new violation and no change in the import control file that specifies any of the new imports are truly 'ok' since it is a blanket allow. When we create new sub-packages or include new tools, we will have to manually re-examine and update the import control file. With whitelist, there will be a violation always and the import control file must be modified when a new import is seen and this allows reviewers like us to confirm that the new import isn't breaking anything and is ok to proceed. The only time I have had issues with allows/disallows being intermingled was with https://github.com/checkstyle/checkstyle/issues/5642 in whitelist mode. You also just need to ensure what you are whitelisting isn't too strong, like allowing too broad a range of packages.

for all except for API. only in API ... use while-list model

This still leaves us with the issue of a global blacklist. Instead of with all sub-packages, you are just moving it to API sub-package. The sub-package will have allows that can override the main package's disallows. This still means we would have to duplicate the disallows and reasons to the sub-package to achieve a global blacklist.

[romani]

Can existing local-only=false help here ?

[rnveach]

If I understand what you are asking, local-only=false is the default option for all rules and doesn't make the rule global. See https://github.com/checkstyle/checkstyle/blob/90f20e09869c71eb22190ad9c1d46d5deec324a5/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java#L146

All rules still follow the sub-package hierarchy first regardless of any rule attributes. local-only's real purpose is just for setting it to true for now.

[romani]

Code is funny

From DTD :

local-only - Indicates that the rule is to apply only to the current package and not to subpackages local-only (true) #IMPLIED

So at least we found a bug in loading of config or DTD.

Does it sound like bug that this property does not do what it supposed to do ?

In my mind if local-only=false then rule should be applied to all subpackages

[rnveach]

Does it sound like bug that this property does not do what it supposed to do ? In my mind if local-only=false then rule should be applied to all subpackages

Not a bug and it does apply to all sub-packages, it is just not the first thing that is checked via the rules.

ImportControl uses a bottom first approach (sub-packages before package). local-only still keeps the allow/disallow rule inside the package it is defined. The order of precedence still checks the rules of the subpackage first before the package. Nothing changes that sub-packages are looked at first.

allow local-only=false - a package or sub-package can use this import, but sub-package rules take precedence over this one allow local-only=true - only a package can use this import, but sub-package rules still take precedence

We would need a whole new attribute that would copy allow/disallow imports into all the sub-packages and be examined first, hence a global list. We can either use global=true/false or we can change local-only into a 3 value attribute.

With global on, it would turn:

<package="com.company">
  <disallow class="my.test" global="true"/>
  <subpackage="example">
  </subpackage>
  <subpackage="example2">
  </subpackage>
<package>

into:

<package="com.company">
  <disallow class="my.test"/>
  <subpackage="example">
    <disallow class="my.test"/>
  </subpackage>
  <subpackage="example2">
    <disallow class="my.test"/>
  </subpackage>
<package>

Basically turning this one disallow into a top first (package before sub-packages)

[rnveach]

@romani Here is an example of the precedence of import controls.

1)

<package="A">
  <disallow class="X"/> <!-- local-only=false -->
  <subpackage="B"> <!-- delegate to parent by default -->
    <disallow class="Y"/> <!-- local-only=false -->
  </subpackage>
<package>

--->

if (B or subpackageOf(B)) {
  if (Y) exit Y;
}
if (A or subpackageOf(A) or B or subpackageOf(B)) {
  if (X) exit X;
}

2)

<package="A">
  <disallow class="X" local-only="true"/>
  <subpackage="B"> <!-- delegate to parent by default -->
    <disallow class="Y"  local-only="true"/>
  </subpackage>
<package>

--->

if (B and !subpackageOf(B)) {
  if (Y) exit Y;
}
if (A and !subpackageOf(A) and !B and !subpackageOf(B)) {
  if (X) exit X;
}

3)

<package="A">
  <disallow class="X"/> <!-- local-only=false -->
  <subpackage="B" strategyOnMismatch="allowed">
    <disallow class="Y"/> <!-- local-only=false -->
  </subpackage>
<package>

--->

if (B or subpackageOf(B)) {
  if (Y) exit Y;
  exit allowed;
}
if (A or subpackageOf(A)) {
  if (X) exit X;
}

deepest sub-packages always come first. allows/disallows are in the same order they are defined. sub-packages are only allowed to leave to the parent package if their strategy on mismatch is delegate. local-only=true just means the current import control package must match the package in the file exactly.

[romani]

Good points for documentation update.

Looks like we need some attribute to let parents subpackages apply some rule to all children. New attribute for-all-subpackages=true|false false is default. Rules will be copied to all subpackages controls, it will be same as do copy manually.

Should it help to your use case ?

[rnveach]

Should it help to your use case ?

Yes, that would create a global blacklist or allowlist.

[romani]

I recommend to close this issue (too much discussions in it ) and make two more: To fix documentation for local-only To introduce for-all-subpackages to support global blacklist or allowlist.