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

[SR-1903] Method in TextCheckingResult conflicts with property #4561

Open
swift-ci opened this issue Jun 26, 2016 · 14 comments
Open

[SR-1903] Method in TextCheckingResult conflicts with property #4561

swift-ci opened this issue Jun 26, 2016 · 14 comments

Comments

@swift-ci
Copy link
Contributor

Previous ID SR-1903
Radar None
Original Reporter rdemarest (JIRA User)
Type Bug
Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug, SDKOverlay
Assignee None
Priority Medium

md5: 9c19171050fa9a944374dfec0cf34d98

Issue Description:

In (NS)TextCheckingResult class we have a property named range:

public var range: NSRange { get }

but also a method named range:

public func range(at idx: Int) -> NSRange

The latter is due to the name translation from ObjC to Swift. This conflicts with the property, if you try to use the function like so:

if match.range(at: MatchGroup.localVariable).location != NSNotFound {

You get the follow error: "Cannot call value of non-function type 'NSRange' (aka '_NSRange')"

Because it thinks I'm trying to use the property rather than the method.

@belkadan
Copy link

cc @phausler

@phausler
Copy link
Member

Both of these methods are appropriately named per their usage and the naming rules.

@property (readonly) NSRange range;

- (NSRange)rangeAtIndex:(NSUInteger)idx NS_AVAILABLE(10_7, 4_0);

The only way to fix this is to rename one of them to be more verbose. Perhaps we should change rangeAtIndex to be:

public subscript(_ index: Int) -> Range<Int>

That would be the only case that would make it more "swift-like" and avoid renaming to things that break the naming rules. However we would need to have that be a apinotes change so that sub-classers would get the correct behavior.

@belkadan
Copy link

I'm not sure I'd agree that the members are appropriately named, even if Swift did have better support for overloading methods and properties. They have the same base name, meaning they're part of the same family, but it's not clear what's different between the two of them.

@phausler
Copy link
Member

They are of the same family; range returns the primary range of the match and range(at: n) returns the nth range. TextCheckingResult provides the promise that there is at least one range stored.

@belkadan
Copy link

Okay.

@DougGregor, do you know what we'd change to allow this?

@swift-ci
Copy link
Contributor Author

Comment by Remy Demarest (JIRA)

What's interesting with this property is that it's basically a convenience accessor for the method:

result.range == result.range(at: 0)

@phausler
Copy link
Member

Will changing the name via the apinotes via

  - Selector: 'rangeAtIndex:'
    SwiftName: subscript(_:)
    MethodKind: Instance

Work? Or is there some other syntax that we have to use?

@phausler
Copy link
Member

We will need to maintain the base method since that is a required subclasser method for behavior when handed back to objective-c apis.

@belkadan
Copy link

We don't currently allow turning methods into subscripts, and I don't know if that's something we want to add, especially not in a hurry. I'm also not convinced that that's the right API either.

@phausler
Copy link
Member

Well I am certain that any change here should go through API review from the owners of NSTextCheckingResult first anyhow.

@DougGregor
Copy link
Member

I'm surprised the expression type checker isn't handling this overloading. It should be able to.

@karwa
Copy link
Contributor

karwa commented Jun 28, 2016

Are you sure about this? Here's a snippet of code I'm using right now without issues; it uses both the .range and .range(at🙂 APIs:

(This is using Swift 79446c464648e4a553386a3f9d2fcde41159e6d0)

extension RegularExpression : Matchable {

    public func match(in string: String, range: Range<String.Index>) -> Match? {

        var result : Match?
        enumerateMatches(in: string, options: [], range: string.toNSRange(range: range)) {
            match_result, flags, should_stop in

            defer { should_stop.pointee = true } // We only care about the first result
            guard let match_result = match_result else { return }

            let totalRange   = string.fromNSRange(nsRange: match_result.range)

            var groupMatches = [Int:Range<String.Index>]()
            for groupIdx in 1..<match_result.numberOfRanges {
                let groupRange = match_result.range(at: groupIdx)
                guard groupRange.location != NSNotFound else { continue }
                groupMatches[groupIdx - 1] = string.fromNSRange(nsRange: groupRange)
            }

            result = Match(range: totalRange, groupMatches: groupMatches)
        }
        return result
    }
}

However, I do have occasional problems with Array's `.first` and `.first(where: )`, so maybe it's more nuanced than that.

@phausler
Copy link
Member

phausler commented Jul 5, 2016

The case presented here actually uses the strict typing that forces the correct disambiguation. Trial let operations will unfortunately cause a failure because the property is named the same as the function family name.

E.g.

let p = result.range // this is ambiguous
let q: NSRange = result.range // this picks the "right" overload on the return result

@karwa
Copy link
Contributor

karwa commented Jul 8, 2016

Hmmm... that same code snippet is causing problems on a newly-built toolchain, but not Xcode 8 beta 2's one.

EDIT: Oh, it was renamed to `rangeAt`. I think we should solve this in a better way; it crops up in quite a few places and as Doug said, the type-checker really should be able to solve this overloading.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants