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

Fixes for Swift 5.7 compiler #4823

Merged
merged 3 commits into from
Aug 28, 2022

Conversation

lukeredpath
Copy link
Contributor

@lukeredpath lukeredpath commented May 13, 2022

Issue Link 🔗

I'm currently experimenting with the latest development Swift 5.7 toolchain and the Charts library was not compiling due to a bug in the implementation of RangeReplaceableCollection on ChartDataItem.

This PR fixes that by adding the missing protocol method and also fixes a couple of compiler warnings (which I think also exist on the latest stable toolchain).

More info on this here: swiftlang/swift#58737 - the reason this compiled pre-5.7 was due to a bug in Swift which was due to be fixed in 5.6, reverted and will ship in 5.7 by the looks of it.

Goals ⚽

Library can now be compiled with Swift 5.7.

Testing Details 🔍

Library continues to compile under 5.6.

I wasn't able to get the snapshots to pass on my M1 Max laptop - maybe I'm using the wrong simulator. The failures were very subtle and I don't think are related to this change.

@@ -31,6 +31,7 @@ DerivedData
*.ipa
*.dSYM.zip
*.dSYM
*.dia
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files get spewed out when compiling with the development toolchain so I added them to the ignore file.

"revision": "6b24333510e9044cf4716a07bed65eeed6bc6393",
"version": "0.0.8"
"revision": "0a5bc04095a675662cf24757cc0640aa2204253b",
"version": "1.0.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Xcode updated these automatically and I think its a reasonable update as they were very outdated.

entries.replaceSubrange(subrange, with: newElements)
notifyDataSetChanged()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the original issue here? It has been too long; sorry

I would like to get this merged asap, so bear with me my direct asking

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also - if we are compiling with old Swift, like 5.0 or 5.4, will this cause a regression? Not fulling understanding the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of the original bug is that you've always needed replaceSubrange to conform to RangeReplaceableCollection but the compiler wasn't failing when it was missing until 5.7 fixed this issue. You'd need to compile with an older toolchain to be sure, I don't know how far back you want to support in terms of toolchain.

Copy link
Member

@liuxuan30 liuxuan30 Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically we usually just support latest one; but given this issue is totally different between 5.6/5.7, I'm not wure what would happen in the field.

Would @avaialble(iOS 16) , (macOS, 13) or something similar solve it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to compile on Xcode beta3, howerver, it still throws

Type 'ChartDataSet' does not conform to protocol 'RangeReplaceableCollection'
Unavailable instance method 'replaceSubrange(_:with:)' was used to satisfy a requirement of protocol 'RangeReplaceableCollection'

which Xcode beta you are building?

@@ -175,7 +175,7 @@ extension CGContext
}
}

open func drawText(_ text: String, at point: CGPoint, anchor: CGPoint = CGPoint(x: 0.5, y: 0.5), angleRadians: CGFloat, attributes: [NSAttributedString.Key : Any]?)
public func drawText(_ text: String, at point: CGPoint, anchor: CGPoint = CGPoint(x: 0.5, y: 0.5), angleRadians: CGFloat, attributes: [NSAttributedString.Key : Any]?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with the Xcode suggested fix here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the error message for open?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember now, sorry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the error message for open?

quoted from this issue:

Build error around CandleStickChartRenderer's and RadarChartRenderer's drawData(context: CGContext) where the IndexingIterator are not equivalent. Raw message:

error build: Referencing instance method 'makeIterator()' on 'Collection' requires the types 'IndexingIterator' and 'IndexingIterator' be equivalent

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quoted issue doesn't seem to be relevant here. For me, the issue

CandleStickChartRenderer.swift:41:59: error build: Referencing instance method 'makeIterator()' on 'Collection' requires the types 'IndexingIterator<ChartData>' and 'IndexingIterator<CandleChartData>' be equivalent

still was still there with this PR's branch. It was gone only after I made the required conversions: radarData as ChartData in RadarChartRenderer:55 and candleData as ChartData at CandleStickChartRenderer:41

I am using Xcode 14 beta 3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The open vs public is not an error, but a build warning:

Non-'@objc' instance method in extensions cannot be overridden; use 'public' instead

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quoted issue doesn't seem to be relevant here. For me, the issue

CandleStickChartRenderer.swift:41:59: error build: Referencing instance method 'makeIterator()' on 'Collection' requires the types 'IndexingIterator<ChartData>' and 'IndexingIterator<CandleChartData>' be equivalent

still was still there with this PR's branch. It was gone only after I made the required conversions: radarData as ChartData in RadarChartRenderer:55 and candleData as ChartData at CandleStickChartRenderer:41

I am using Xcode 14 beta 3

This is the way!

@lukeredpath lukeredpath marked this pull request as ready for review May 13, 2022 23:26
@Brett-Best Brett-Best mentioned this pull request Jul 7, 2022
@dangthaison91
Copy link

I also face other compiler errors:

    internal static let EaseOutBack = { (elapsed: TimeInterval, duration: TimeInterval) -> Double in
        let s: TimeInterval = 1.70158
        var position: TimeInterval = elapsed / duration
        position -= 1.0
        return Double( position * position * ((s + Double(1.0)) * position + s) + Double(1.0) )
    }
        for case let set as RadarChartDataSetProtocol in (radarData as ChartData) where set.isVisible
        {
            drawDataSet(context: context, dataSet: set, mostEntries: mostEntries)
        }
        for case let set as CandleChartDataSetProtocol in (candleData as ChartData) where set.isVisible
        {
              drawDataSet(context: context, dataSet: set)
        }

from: #4860 (comment)

dangthaison91 pushed a commit to dangthaison91/Charts that referenced this pull request Jul 26, 2022
dangthaison91 pushed a commit to dangthaison91/Charts that referenced this pull request Aug 2, 2022
dangthaison91 pushed a commit to dangthaison91/Charts that referenced this pull request Aug 2, 2022
JackMoseley2001 pushed a commit to JackMoseley2001/ChartsKit that referenced this pull request Aug 2, 2022
mustiikhalil pushed a commit to fishbrain/Charts that referenced this pull request Aug 11, 2022
@o15a3d4l11s2
Copy link

Any update when this could be reviewed and merged?

grahamburgsma pushed a commit to ConstantHealth/Charts that referenced this pull request Aug 16, 2022
@liuxuan30
Copy link
Member

liuxuan30 commented Aug 25, 2022

Any update when this could be reviewed and merged?

my comments above haven't resolved yet, not sure yet. I tried this PR with beta 3, however couldn't build & run.

Probably the latest time is when Xcode GM released and I can try it without bothering from beta releases.

tasostsolis pushed a commit to tasostsolis/Charts that referenced this pull request Aug 26, 2022
@pmairoldi pmairoldi self-assigned this Aug 27, 2022
@pmairoldi pmairoldi merged commit 1ae0003 into ChartsOrg:master Aug 28, 2022
@pmairoldi
Copy link
Collaborator

Thanks for contributing!

@leohidalgo
Copy link

@pmairoldi do you have a deadline for a new version?

@pmairoldi
Copy link
Collaborator

Waiting for Xcode 14 gm before releasing anything.

@leohidalgo
Copy link

@pmairoldi The GM is here, let me know if I can help you with anything.

@lukeredpath lukeredpath deleted the lukeredpath/swift-5.7 branch September 7, 2022 23:31
@liuxuan30
Copy link
Member

Waiting for Xcode 14 gm before releasing anything.

thanks pal! been busy with the new job, as we also had tons of issues regarding Xcode 14 RC and iOS 16

pinkpika pushed a commit to cmmobile/Charts that referenced this pull request Oct 5, 2022
mk-wang pushed a commit to mk-wang/Charts that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet