Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Sep 5, 2023

Fixes: #1142
Context: #1130

In #1130 we added a feature to automatically mark a method as "deprecated" if it overrides a "deprecated" method.

However, there can be instances where this is undesirable for a user, and there is currently no way to opt out on a global or metadata level.

Add a global opt-out for this feature, usable via:

--lang-features=do-not-fix-obsolete-overrides

This will be made available to users via an MSBuild property in an additional PR.

@jpobst jpobst force-pushed the obsolete-overrides-toggle branch from f27ca48 to 14dd30b Compare September 5, 2023 15:46
@jpobst jpobst marked this pull request as ready for review September 5, 2023 16:00
v => opts.ApiLevel = v },
{ "lang-features=",
"For internal use. (Flags: interface-constants,default-interface-methods,nested-interface-types,nullable-reference-types,obsoleted-platform-attributes,restrict-to-attributes)",
"For internal use. (Flags: interface-constants,default-interface-methods,nested-interface-types,nullable-reference-types,obsoleted-platform-attributes,restrict-to-attributes,dont-fix-obsolete-overrides)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call it do-not-fix-obsolete-overrides instead of dont-…?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good call.

@jonpryor
Copy link
Contributor

jonpryor commented Sep 11, 2023

I do not like that this is a "global" flag. It feels like the real problem is that #1130 doesn't support "un-deprecation". I assume that we have Java-side code akin to:

package android.app;
@Deprecated // added API-30
/* partial */ class IntentService {
    @Deprecated // added API-30
    public android.os.IBinder onBind(android.content.Intent intent) {…}
}

package com.microsoft.intent.mam.client.app;
// NOT @Deprecated
/* partial */ class MAMIntentService extends IntentService {
    @Override
    public final android.os.IBinder onBind(android.content.Intent intent) {…}
}

Which is unfortunately ~identical to the scenario mentioned in #1130:

public class ActionProvider {
    @Obsolete // since API-16
    public View onCreateActionView() {…}
}
public class MediaRouteActionProvider extends ActionProvider {
    // no @Obsolete
    public View onCreateActionView() {…}
}

Part of the problem is that "un-deprecation" is an odd construct in itself; it isn't A Thing™.

Maybe a global flag is the right approach?

I do wonder if there's a "scoped" solution, though. Perhaps we could only "auto-deprecate overrides" for methods in the same binding project? If possible, this would also fix #1142 because the MAM .jar is not in Mono.Android.dll

Alternatively, perhaps we could introduce an @unobsolete XML attribute to not automatically deprecated that particular override?

@jpobst
Copy link
Contributor Author

jpobst commented Sep 11, 2023

In general, I don't think this flag will ~ever be used. As you say, un-deprecating a method isn't a thing.

However, this feature did create a scenario that cannot be fixed with metadata or any other mechanism, which is undesirable.

For a resolution that I suspect will not get ~any use, I went with what seemed easier to implement: the global flag. 🤷‍♂️

@jonpryor jonpryor merged commit 8e63cc8 into main Oct 2, 2023
@jonpryor jonpryor deleted the obsolete-overrides-toggle branch October 2, 2023 19:45
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Java bindings incorrectly assigning method as obsolete

3 participants