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-88] Remove old mirrors #42710

Open
gribozavr opened this issue Dec 6, 2015 · 23 comments
Open

[SR-88] Remove old mirrors #42710

gribozavr opened this issue Dec 6, 2015 · 23 comments
Labels
affects ABI Flag: Affects ABI standard library Area: Standard library umbrella task

Comments

@gribozavr
Copy link
Collaborator

Previous ID SR-88
Radar rdar://problem/21622060
Original Reporter @gribozavr
Type Task
Status In Progress
Resolution
Additional Detail from JIRA
Votes 0
Component/s Standard Library
Labels Task, AffectsABI
Assignee austin (JIRA)
Priority Medium

md5: 084e23cc744db09ed5ef4c17b5e5fe0f

is blocked by:

  • SR-392 Creating a String from an object of type ErrorType segfaults
  • SR-426 Memory leak with enum containing class, downcast to Any.Type

Issue Description:

We need to remove old mirror API: the _Reflectable protocol and all related underscored APIs.

Plan:

1. All old mirrors in the standard library and SDK overlays should be migrated to the new CustomReflectable APIs. All mirrors that didn't have tests and are affected by this conversion should get tests.

2. dump() function should be ported to use the new mirrors.

3. The new mirror API should be migrated off the old _reflect() API. The issue is that _reflect() function and its implementation details depend on the old mirrors. We need to design new runtime entry points that make sense for the new mirrors implementation. This part requires writing a proposal, because the runtime API is going to be stable.

4. Remove old underscored mirrors. Remove old runtime entry points.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 6, 2015

Comment by Austin Zheng (JIRA)

Not sure what the convention for this Jira instance is, so I'll tentatively assign this to myself and begin working on it (at least #1 and #2 for now)? If that's not the right thing to do feel free to un/reassign.

@gribozavr
Copy link
Collaborator Author

Thanks, Austin!

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 7, 2015

Comment by Austin Zheng (JIRA)

A cursory search in Xcode seems to indicate that _UTF8ViewMirror isn't declared anywhere in the codebase, although it should be a nested type within String. I'll keep looking later.

@gribozavr
Copy link
Collaborator Author

_UTF8ViewMirror is generated in stdlib/public/core/StringUTFViewsMirrors.swift.gyb.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 7, 2015

Comment by Austin Zheng (JIRA)

Ah, thanks. I'll be sure to check the gyb templates the next time this happens.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 7, 2015

Comment by Austin Zheng (JIRA)

For tracking purposes, @jckarter mentioned on the lists he was working on #3.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 8, 2015

Comment by Austin Zheng (JIRA)

I'm planning to modify the .gyb and .py files related to generating _Mirror boilerplate. If you'd rather me not let me know.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 8, 2015

Comment by Austin Zheng (JIRA)

I have a question. I notice that _Reflectable encompasses both CustomReflectable and CustomPlaygroundQuickLookable's functionality. It looks like there are a couple of _MirrorTypes that exist only to provide quicklook functionality.

Can I assume that, quicklook behavior aside, a _MirrorType conformer that is hardcoded to report zero children can be replaced by the default runtime mirror (e.g. the type who the _FoobarMirrorType belongs to doesn't need to conform to CustomReflectable)?

If you don't know the answer I'll go through Reflection.mm and try to piece out an answer. Posting this just in case Dmitri or Joe knows off the top of their head.

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 8, 2015

Comment by Austin Zheng (JIRA)

Final question for tonight: is there an explicit replacement for _MirrorType's "summary" property? As-is, the current Mirror type provides no analogous property, and in most cases the existing custom mirrors just return the object's description. (I need to know this in order to properly update the unit tests.) Thanks!

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

Still working on this. Hope to have a PR by end of weekend.

@gribozavr
Copy link
Collaborator Author

Thanks, Austin!

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

Finished with the conversion parts of 1 and 2. I need to update/get existing reflection-related tests to run correctly, and increase test coverage.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

I'll need a few more days. Manual testing indicate mirrors seem to be working correctly. Still converting unit tests (lots of .summary), fixing bugs, writing new ones.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

I haven't dropped this. Still debugging issues with my implementation.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

I have most, but not all the tests passing. There's a leak with the generic singleton mirror test (and a few others) I need to troubleshoot.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

I narrowed down the cause of the failing unit tests to the following, linked issue (which can be reproduced without using mirrors, reflection, or anything specific to the stdlib).

@gribozavr
Copy link
Collaborator Author

Thank you! Could you bring up these issues on the mailing list, CC'ing @jckarter?

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 1, 2016

Comment by Austin Zheng (JIRA)

PR here: #838 Happy 2016!

I understand it's quite big for a PR, but many of the changes are straightforward. I'm happy to break it up if you'd prefer. Preferably a few people can try reviewing it to see if the code conforms to expectations etc.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

I have some time this weekend; I'll get in an updated PR in the next few days per comments on the GitHub thread.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

Hello. Dave Abrahams merged my PR (#838 which means #1 and #2 are mostly done. Let me know if I can help with the other steps.

@swift-ci
Copy link
Collaborator

Comment by Austin Zheng (JIRA)

Phases #1 and #2 are actually done now (revised PR merged). @jckarter, if there's anything I can do to help with #3 or #4 let me know.

@swift-ci
Copy link
Collaborator

swift-ci commented Jun 4, 2016

Comment by Austin Zheng (JIRA)

I assume #3 is going to be deferred until Swift 3.x.

@Dante-Broggi
Copy link
Contributor

Is this resolved, or no longer valid? If either, this should be closed.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects ABI Flag: Affects ABI standard library Area: Standard library umbrella task
Projects
None yet
Development

No branches or pull requests

3 participants