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

Allow multiple colons in selectors #336

Merged
merged 6 commits into from
Jan 13, 2023
Merged

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Jan 12, 2023

Should help cutting down on the amount of work #327 and futures has to do. Tagging @silvanshade.

@madsmtm madsmtm added A-framework Affects the framework crates and the translator for them bug Something isn't working labels Jan 12, 2023
@madsmtm madsmtm force-pushed the multiple-colons-in-selectors branch 2 times, most recently from 26eb66b to 983633e Compare January 12, 2023 06:03
Base automatically changed from update-icrate-sdk to master January 12, 2023 06:05
@madsmtm madsmtm force-pushed the multiple-colons-in-selectors branch from 983633e to 9cc6288 Compare January 12, 2023 06:06
@silvanshade
Copy link
Contributor

Thanks for starting on this. I was taking a look at the changes and although it seems like it should fix some of the issues with methods that currently need to be skipped there may still be some problems that remain.

When I was trying this PR locally, I still ran into problems with methods that get translated as duplicate names. For instance, we have situations like this:

extern_methods!(
    #[cfg(feature = "WebKit_WebView")]
    unsafe impl WebView {
        #[method(goForward)]
        pub unsafe fn goForward(&self) -> bool;
    }
);

extern_methods!(
    /// WebIBActions
    #[cfg(feature = "WebKit_WebView")]
    unsafe impl WebView {
        #[method(goForward:)]
        pub unsafe fn goForward(&self, sender: Option<&Object>);
    }
);

I also notice that in the examples that now translate in the PR, all of the trailing : are collapsed:

    unsafe impl CAMediaTimingFunction {
        #[method_id(@__retain_semantics Other functionWithControlPoints::::)]
        pub unsafe fn functionWithControlPoints(
            c1x: c_float,
            c1y: c_float,
            c2x: c_float,
            c2y: c_float,
        ) -> Id<Self, Shared>;
    }

So it seems we may run into a general issue with disambiguation, say if we have a selector like foo:bar and another one like foo::bar, and then again similarly for selectors with prefixed or suffixed colons.

I guess the problem with just always naively translating : as _ in the method name is that it looks kind of ugly since so many of these methods would now end with a trailing _, and also the reserved keywords are translated as k -> k_ so then that becomes an additional source of ambiguity.

Maybe an alternative approach that would resolve these potential issues would be to adopt a strategy like the following:

  1. translate keywords using only raw identifiers (e.g., type -> r#type)
  2. if there are multiple methods m0 and m1 that would be translated to the same method name, disambiguate them by translating all of the instances of : directly as _ (including multiple subsequent _ if necessary)

(2) should not be too difficult, but one potential downside is that it could lead to an additional source of breakage with regard to #338 if a new ambiguating method gets added in an updated SDK.

Another possibility might be to allow specifying a particular renaming for a given selector directly in the translation config file and then just manually patching up these cases. I suppose the ambiguous cases are probably rare enough that that would suffice and would maybe lead to nicer names anyway (at the potential of some slight confusion, at least until or unless the API docs are linked into the generated output).

Any thoughts on whether this might work or if there is a better way to handle these issues in general?

@madsmtm
Copy link
Owner Author

madsmtm commented Jan 13, 2023

translate keywords using only raw identifiers

That's a great idea, we should definitely do that!

if there are multiple methods m0 and m1 that would be translated to the same method name, disambiguate them by translating all of the instances of : directly as _ (including multiple subsequent _ if necessary)

We could use some kind of ambiguity detection anyhow for duplicate class & instance methods. I would guess it would be easiest as a post-processing step in cache.rs, if you want to try have a stab at it?

it could lead to an additional source of breakage with regard to #338 if a new ambiguating method gets added in an updated SDK.

Good point, though probably quite unlikely (and if it's the case, we can fix it manually just like the other breakage there).

allow specifying a particular renaming for a given selector

I've been considering this anyhow in #330, and will also be necessary when we start doing #284.


Really, I'm unsure of how common it is that method names are duplicated, which is kind of required knowledge to figure out which fix is the better solution? Perhaps one could run a test script against all frameworks in /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk, and figure it out? But then again, it might just be easier to fix the problem properly and never again worry about it.

@madsmtm madsmtm merged commit 09d117d into master Jan 13, 2023
@madsmtm madsmtm deleted the multiple-colons-in-selectors branch January 13, 2023 23:59
@madsmtm
Copy link
Owner Author

madsmtm commented Jan 14, 2023

We could probably also use proc_macro2 or quote for translating keywords to identifiers

@silvanshade
Copy link
Contributor

We could use some kind of ambiguity detection anyhow for duplicate class & instance methods. I would guess it would be easiest as a post-processing step in cache.rs, if you want to try have a stab at it?

Okay, I will give it a shot.

I did briefly try to modify the translation of the selectors earlier, specifically removing the trimming of trailing colons here but after doing that and running the translator again it almost immediately errored out with some problem about argument analysis IIRC.

But maybe a post processing step in cache.rs should avoid this?

@madsmtm
Copy link
Owner Author

madsmtm commented Jan 14, 2023

Perhaps because after the function name changed, ClassData::get_method_data in stmt.rs now fails finding the method skips that we've declared in translation-config.toml?

So yeah, I think a post-process step will avoid this.

@madsmtm madsmtm added the A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates label Jan 27, 2023
@madsmtm madsmtm added this to the icrate v0.1.0 milestone Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Affects the framework crates and the translator for them A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants