Kevin, I really like your reasoning about avoiding the code churn, and finding a better way to flag something to avoid optional hoisting. I think its better solution to the problem. It also has the added benefit of kicking out to map() and getting a slightly optimised version of the same method - free speed win for devs who unknowingly used `flatMap` not realizing `map()` is the preferred solution.
I’m not sure the potential for confusion is enough to add a full new attribute flag for “Warning” though. It’s a minor win with a large relative overhead, and explaining in the deprecation warning that flat maps that act like direct maps are not recommended is reasonable enough. - Rod > On 9 Nov 2017, at 5:43 am, John McCall via swift-evolution > <swift-evolution@swift.org> wrote: > > >> On Nov 8, 2017, at 1:20 PM, Kevin Ballard <ke...@sb.org >> <mailto:ke...@sb.org>> wrote: >> >> On Tue, Nov 7, 2017, at 09:37 PM, John McCall wrote: >>> >>>> On Nov 7, 2017, at 6:34 PM, Kevin Ballard via swift-evolution >>>> <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote: >>>> >>>> On Tue, Nov 7, 2017, at 03:23 PM, John McCall via swift-evolution wrote: >>>>> https://github.com/apple/swift-evolution/blob/master/proposals/0187-introduce-filtermap.md >>>>> >>>>> <https://github.com/apple/swift-evolution/blob/master/proposals/0187-introduce-filtermap.md> >>>>> >>>>> • What is your evaluation of the proposal? >>>> >>>> This proposal is going to cause an insane amount of code churn. The >>>> proposal suggests this overload of flatMap is used "in certain >>>> circumstances", but in my experience it's more like 99% of all flatMaps on >>>> sequences are to deal with optionals, not to flatten nested sequences. >>>> >>>>> • Is the problem being addressed significant enough to warrant a change >>>>> to Swift? >>>> >>>> I don't think so. It's a fairly minor issue, one that really only affects >>>> new Swift programmers anyway rather than all users, and it will cause far >>>> too much code churn to be worthwhile. >>>> >>>> I'd much rather see a proposal to add a new @available type, something >>>> like 'warning', that lets you attach an arbitrary warning message to a >>>> call (which you can kind of do with 'deprecated' except that makes the >>>> warning message claim the API is deprecated). >>> >>> As review manager, I generally try to avoid commenting on threads, but I >>> find this point interesting in a way that, if you don't mind, I'd like to >>> explore. >>> >>> Would this attribute not be a form of deprecation? Certainly it acts to >>> discourage current and subsequent use, since every such use will evoke a >>> warning. >>> >>> Is the word "deprecation" just too strong? Often we think of deprecated >>> APIs as being ones with more functional problems, like an inability to >>> report errors, or semantics that must have seemed like a good idea at the >>> time. Here it's just that the API has a name we don't like, and perhaps >>> "deprecation" feels unnecessarily judgmental. >> >> What I'm suggesting is that we don't change the API name at all. That's why >> I don't want to use 'deprecated', because we're not actually deprecating >> something. I'm just suggesting an alternative way of flagging cases where >> the user tries to use flatMap but accidentally invokes optional hoisting, >> and that's by making a new overload of flatMap that works for non-optional >> (non-sequence) values and warns the user that what they're doing is better >> done as a map. Using the 'deprecated' attribute for this would be confusing >> because it would make it sound like flatMap itself is deprecated when it's >> not. > > I see. Thanks. > > John. > >> >>> Also, more practically, it conflates a relatively unimportant suggestion — >>> that we should call the new method in order to make our code clearer — with >>> a more serious one — that we should revise our code to stop using a >>> problematic API. Yes, the rename has a fix-it, but still: to the extent >>> that these things demand limited attention from the programmer, that >>> attention should clearly be focused on the latter set of problems. Perhaps >>> that sense of severity is something that an IDE should take into >>> consideration when reporting problems. >>> >>> What else would you have in mind for this warning? >> >> The main use for this warning would be for adding overloads to methods that >> take optionals in order to catch the cases where people invoke optional >> hoisting, so we can tell them that there's a better way to handle it if they >> don't have an optional. flatMap vs map is the obvious example, but I'm sure >> there are other cases where we can do this too. >> >> But there are also other once-off uses. For example, in the past I've >> written a function that should only ever be used for debugging, so I marked >> it as deprecated with a message saying 'remove this before committing your >> code'. This warning would have been better done using the new 'warning' >> attribute instead of as a deprecation notice. >> >> -Kevin Ballard >> >>> John. >>> >>>> With that sort of thing we could then declare >>>> >>>> extension Sequence { >>>> @available(*, warning: "Use map instead") >>>> func flatMap<U>(_ f: (Element) -> U) -> [U] { >>>> return map(f) >>>> } >>>> } >>>> >>>> And now if someone writes flatMap in a way that invokes optional hoisting, >>>> it'll match this overload instead and warn them. >>>> >>>>> • How much effort did you put into your review? A glance, a quick >>>>> reading, or an in-depth study? >>>> >>>> A quick reading, and a couple of minutes testing overload behavior with >>>> availability attributes (to confirm that we can't simply use 'unavailable' >>>> for this). >>>> >>>> -Kevin Ballard >>>> >>>> _______________________________________________ >>>> swift-evolution mailing list >>>> swift-evolution@swift.org <mailto:swift-evolution@swift.org> >>>> https://lists.swift.org/mailman/listinfo/swift-evolution >>>> <https://lists.swift.org/mailman/listinfo/swift-evolution> >> > > _______________________________________________ > swift-evolution mailing list > swift-evolution@swift.org > https://lists.swift.org/mailman/listinfo/swift-evolution
_______________________________________________ swift-evolution mailing list swift-evolution@swift.org https://lists.swift.org/mailman/listinfo/swift-evolution
