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

explicit_acl and explicit_top_level_acl conflict with no_extension_access_modifier #2694

Closed
samrayner opened this issue Mar 27, 2019 · 14 comments

Comments

Projects
None yet
7 participants
@samrayner
Copy link
Contributor

commented Mar 27, 2019

New Issue Checklist

Describe the bug

In Xcode 10.2 these rules conflict, whereas previous they didn't (although I'm not sure why). It is now not possible to satisfy both of them (which makes sense).

I think the easiest solution would be to add configuration options to explicit_acl and explicit_top_level_acl to exclude extensions?

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint
/Users/samrayner/Development/PhotoKiosk/PhotoKiosk/ViewModels/PhotoCollectionsViewModel.swift:72:1: warning: Explicit ACL Violation: All declarations should specify Access Control Level keywords explicitly. (explicit_acl)
...

Environment

  • SwiftLint version (run swiftlint version to be sure)?
    0.31.0
  • Installation method used (Homebrew, CocoaPods, building from source, etc)?
    Homebrew
  • Paste your configuration file:
opt_in_rules:
  - explicit_acl
  - no_extension_access_modifier
  • Are you using nested configurations?
    No
  • Which Xcode version are you using (check xcode-select -p)?
    10.2
  • Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
    to quickly test if your example is really demonstrating the issue. If your example is more
    complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.
echo "extension String {}" | swiftlint lint --no-cache --use-stdin --enable-all-rules
<nopath>:1:1: warning: Explicit Top Level ACL Violation: Top-level declarations should specify Access Control Level keywords explicitly. (explicit_top_level_acl)
<nopath>:1:1: warning: Explicit ACL Violation: All declarations should specify Access Control Level keywords explicitly. (explicit_acl)
Done linting! Found 2 violations, 0 serious in 1 file.

echo "internal extension String {}" | swiftlint lint --no-cache --use-stdin --enable-all-rules
<nopath>:1:1: error: No Extension Access Modifier Violation: Prefer not to use extension access modifiers (no_extension_access_modifier)
Done linting! Found 1 violation, 1 serious in 1 file.
@fixpunkt

This comment has been minimized.

Copy link

commented Mar 28, 2019

I started getting this after upgrading to Xcode 10.2 today. Fun fact: even after downgrading to Xcode 10.1 (including removing and re-installing the Xcode CLI tools), the issue still persists. What could be causing this?

@jpsim

This comment has been minimized.

Copy link
Collaborator

commented Apr 7, 2019

Seems like we might want to ignore extensions in explicit_top_level_acl?

@akashivskyy

This comment has been minimized.

Copy link

commented Apr 11, 2019

And explicit_acl as well. Currently, if explicit_acl is enabled, it is impossible to satisfy it:

// compiles but violates

internal extension Foo {
  func bar() 
}
// compiles but violates

extension Foo {
  internal func bar() 
}
// compiles but violates

extension Foo {
  func bar() 
}
// doesn't violate but doesn't compile either

internal extension Foo {
  internal func bar() 
}
@svenmuennich

This comment has been minimized.

Copy link

commented Apr 19, 2019

Let's see if #2720 fixes this issue.

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

@svenmuennich it doesn't now it is

@jpsim jpsim referenced this issue Apr 29, 2019

Merged

Swift 5 support #2720

@jpsim

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

Fixed in #2720 by @marcelofabri

@jpsim jpsim closed this Apr 29, 2019

@samrayner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Thank you guys!

@Noobish1

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Sorry to comment on a closed issue but the top example from @akashivskyy doesn't seem to work in 0.32.0/Swift 5, is this expected?

@jpsim

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

What do you mean @Noobish1?

$ swift --version  
Apple Swift version 5.0 (swiftlang-1001.0.69.5 clang-1001.0.46.3)
Target: x86_64-apple-darwin18.6.0
$ swiftlint version
0.32.0
$ echo "extension String {}" | swiftlint lint --no-cache --use-stdin --enable-all-rules
Done linting! Found 0 violations, 0 serious in 1 file.
@Noobish1

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@jpsim I meant the full example including the function, though this might be a totally separate issue

I have explicit_top_level_acl and explicit_acl enabled.

If I try to write this:

internal extension String {
    internal func someFunc() {}
}

It doesn't compile in Xcode and gives me an error stating:

'internal' modifier is redundant for instance method declared in an internal extension

If I then use the fix it and remove the internal ACL from the function I end up with:

internal extension String {
    func someFunc() {}
}

which violates explicit_acl:

Explicit ACL Violation: All declarations should specify Access Control Level keywords explicitly. (explicit_acl)

I'm using Swift 5.0.1 in Xcode 10.2.1

@jpsim

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

How about this?

extension String {
    internal func someFunc() {}
}
@Noobish1

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

ha that does actually work, I guess it only checks when you explicitly declare the ACL for the extension?

I guess that'll do the job, thanks @jpsim, sorry again for commenting on a closed issue.

@akashivskyy

This comment has been minimized.

Copy link

commented May 17, 2019

I think the first case (internal extension, func) leaves us with two unanswered questions:

  1. Should SwiftLint raise violations in cases that don't compile when trying to satisfy them?
  2. Should enabling explicit_acl require to shift access modifiers from extensions to their inner declarations? (this could be seen as a regression when migrating to Swift 5.0)

If one of the answers is yes then we're fine. If both answers are no, then it's a bug that needs to be addressed. WDYT @jpsim?

@jpsim

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

Both answers are yes.

SwiftLint has always assumed that it was running on code that compiles. The alternative would make SwiftLint as complex as the compiler itself.

The explicit ACL rule is an opt-in rule where developers want to make their declarations have an explicit ACL keyword. An extension scope is not a declaration in and of itself. I can’t reference your extension when writing code against your module. Function declarations are declarations.

I hope that helps clarify things.

Can we please open new tickets moving forward if we have follow up discussions? Commenting on closed tickets makes it way more likely to 1) miss important information that is suggested by the new issue template and 2) lead to a maintainer not answering right away and forgetting that a thread was started in a closed ticket. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.