Skip to content

Conversation

atsushieno
Copy link
Contributor

This is a fixup for c51469e, as well as bugfix for:
https://bugzilla.xamarin.com/show_bug.cgi?id=51337

What's happening behind bug #51337 was this:

BINDINGSGENERATOR:  warning BG8800: Unknown parameter
type org.osmdroid.ResourceProxy.bitmap in method GetBitmap
in managed type OsmDroid.DefaultResourceProxyImpl.

This was, because, "bitmap" is all lowercased and was marked as obfuscated
(because, WHY NAME A CLASS ALL IN LOWERCASE!?).

Such classes should not exist, or should be marked as "obfuscated='false'".

However what bug #51337 implies is that people are not going to make
this additional markup for generator improvements.

What this generator change does is then - a hack. A hack to mark "classes
with very short names" as obfuscated, instead of "all lowercase or number".

As commented on c51469e, there isn't good way to check API element
siblings (it can check sibling names every time at all expensive
calculation). So we just count the sibling nodes and if name length is
short enough to fit within the number of classes, we mark as obfucated.

return false;
int idx = name.LastIndexOf ('.');
string last = idx < 0 ? name : name.Substring (idx + 1);
if (threshold <= Math.Pow (26, last.Length - 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the logic/rationale here, as 26**(last.Length-1) is possibly huge.

If last is c, then it's 26**0 or 1, so unless threshold is 0, all types with names of one character will be allowed. Perhaps that's desirable?

If last is ex, then it's 26**1 or 26, so a package would need 27 types for ex to be considered obfuscated. Huh?

If last is bitmap, then it's 26**5, or 11881376, at which point it's fair to say that no types will be considered to be obfuscated, as threshold will always be less than 11881376.

we just count the sibling nodes and if name length is short enough to fit within the number of classes, we mark as obfucated.

I'm not sure that's what this code does -- though I must admit I'm very confused by it -- and if it does do that, I'm not sure that's necessarily useful?

Perhaps a better obfuscation would be "all class names entirely lowercase that are <= 2 characters in length." Do we know of any obfuscated types that are longer than two characters?

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'm stupid at math. What I wanted was, always treat one-letter name as obfuscation result, treat two-letter names as obfuscated only when there are 26+ classes in the package, same for three-letter names only when there are 26^2+ classes, and so on... Therefore the condition should be:

(!(threshold >= Math.Pow (26, length - 1))

Note that this condition is to filter out non-obfuscated case ("return false").

I avoided simple length <= 2 because there can be FooBar.id or FooBar.in (possibly with FooBar.out). IMO that's no-go.

But you're right, that calculating Math.Pow() for lengthy string doesn't make sense (moreover, there's possible overflow...!). It should be trimmed down to, maybe 3? There wouldn't be 17576 types in a package, even if the library contains a lot of automatically generated types (I'm hesitant to set threshold at 100 or 1000 because of possibly generated types in a single package, 1000 is possible).

(The calculation above then covers "id" ? No, but when there are aa, ab, ... ic, id, ie etc. then this "id" wouldn't be distinct. At least we can say we assumed so).

Copy link
Contributor Author

@atsushieno atsushieno Jan 31, 2017

Choose a reason for hiding this comment

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

Seealso: "id" in the real world use. https://developer.android.com/reference/android/R.id.html

@atsushieno atsushieno force-pushed the less-aggressive-obfuscation-marking branch from e9d90f7 to 6f5eacf Compare January 31, 2017 10:20
@jonpryor
Copy link
Contributor

For a heuristic, I'd suggest these as "obfuscated types":

  1. All class names of length <= 2, in which the entire identifier is lowercase, e.g. aa, ab, etc.
  2. All class names that start with zz (lowercase). This would capture e.g. zze, zzma, etc.

@jonpryor
Copy link
Contributor

Seen in email, but not this thread (!):

Seealso: "id" in real world use. https://developer.android.com/reference/android/R.id.html

That is a good point, but the heuristic wouldn't necessarily hit this, because we already special case a class name prefix of R., so R.id would be skipped (unless you remove the R. check, but I don't think that would be a good idea).

@atsushieno
Copy link
Contributor Author

Anything that happened in R or Manifest could happen in any namespace.

@atsushieno
Copy link
Contributor Author

My comment was hidden under "show outdated" link :p #121 (comment)

@atsushieno atsushieno force-pushed the less-aggressive-obfuscation-marking branch from 6f5eacf to 064f598 Compare February 2, 2017 06:34
@jonpryor
Copy link
Contributor

jonpryor commented Feb 3, 2017

This should also include tests. :-)

It looks like it should be possible to use tools/generator/Tests-Core/api.xml, in that modifying that file locally for me to include a <class name="aa" .../> caused aa to be skipped, so I believe that this will work.

@atsushieno atsushieno force-pushed the less-aggressive-obfuscation-marking branch from 064f598 to 7fa09c2 Compare February 7, 2017 11:38
@atsushieno
Copy link
Contributor Author

Tests are added.

This is a fixup for c51469e, as well as bugfix for:
https://bugzilla.xamarin.com/show_bug.cgi?id=51337

What's happening behind bug #51337 was this:

	BINDINGSGENERATOR:  warning BG8800: Unknown parameter
	type org.osmdroid.ResourceProxy.bitmap in method GetBitmap
	in managed type OsmDroid.DefaultResourceProxyImpl.

This was, because, "bitmap" is all lowercased and was marked as obfuscated
(because, WHY NAME A CLASS ALL IN LOWERCASE!?).

Such classes should not exist, or should be marked as "obfuscated='false'".

However what bug #51337 implies is that people are not going to make
this additional markup for generator improvements.

What this generator change does is then - a hack. A hack to mark "classes
with very short names" as obfuscated, instead of "all lowercase or number".

As commented on c51469e, there isn't good way to check API element
siblings (it can check sibling names every time at all expensive
calculation). So we just count the sibling nodes and if name length is
short enough to fit within the number of classes, we mark as obfucated.

As some post-first-commit thought, the name check is done only for
very short names i.e. only those within 3 letters.

Also, the new proguard in Gradle task seems to obfuscates the names as
"zzXXXXX" . They should be marked as obfuscated too.
@atsushieno atsushieno force-pushed the less-aggressive-obfuscation-marking branch from 7fa09c2 to cb88706 Compare February 7, 2017 15:29
@jonpryor jonpryor merged commit 45aff90 into dotnet:master Feb 7, 2017
jonpryor pushed a commit that referenced this pull request Feb 10, 2017
This is a fixup for c51469e, as well as bugfix for:
https://bugzilla.xamarin.com/show_bug.cgi?id=51337

What's happening behind bug #51337 was this:

	BINDINGSGENERATOR:  warning BG8800: Unknown parameter
	type org.osmdroid.ResourceProxy.bitmap in method GetBitmap
	in managed type OsmDroid.DefaultResourceProxyImpl.

This was, because, "bitmap" is all lowercased and was marked as obfuscated
(because, WHY NAME A CLASS ALL IN LOWERCASE!?).

Such classes should not exist, or should be marked as "obfuscated='false'".

However what bug #51337 implies is that people are not going to make
this additional markup for generator improvements.

What this generator change does is then - a hack. A hack to mark "classes
with very short names" as obfuscated, instead of "all lowercase or number".

As commented on c51469e, there isn't good way to check API element
siblings (it can check sibling names every time at all expensive
calculation). So we just count the sibling nodes and if name length is
short enough to fit within the number of classes, we mark as obfucated.

As some post-first-commit thought, the name check is done only for
very short names i.e. only those within 3 letters.

Also, the new proguard in Gradle task seems to obfuscates the names as
"zzXXXXX" . They should be marked as obfuscated too.
jonpryor pushed a commit that referenced this pull request Oct 4, 2021
Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263
Context: c936d09

Changes: dotnet/android-tools@d92fc3e...34e98e2

  * dotnet/android-tools@34e98e2: [build] Allow Assembly "vendorization" (#136)
  * dotnet/android-tools@061bcc2: [build] Import "parent directory.override.targets" (#135)
  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134)
  * dotnet/android-tools@9b658b2: Merge pull request #133 from xamarin/ndk-r23
  * dotnet/android-tools@ff73f92: [build] Use GitInfo to generate $(Version) (#131)
  * dotnet/android-tools@4c2e36c: [Xamarin.Android.Tools.AndroidSdk] Eclipse Adoptium support (#132)
  * dotnet/android-tools@eaec4e3: [Xamarin.Android.Tools.AndroidSdk] More Microsoft Dist JDK Support (#130)
  * dotnet/android-tools@f9c1b0d: [BaseTasks] improve Task settings in AsyncTaskExtensions (#129)
  * dotnet/android-tools@efc9b67: Merge pull request #128 from dellis1972/bumpitybump
  * dotnet/android-tools@40adec0: Bump LibZipSharp and Mono.Unix to latest stable versions,
  * dotnet/android-tools@87acd6b: [Microsoft.Android.Build.BaseTasks] add StrongName  (#127)
  * dotnet/android-tools@02f7ae7: [NDK] Properly detect 64-bit NDK
  * dotnet/android-tools@623332d: Bump LibZipSharp to 2.0.0-alpha7 (#126)
  * dotnet/android-tools@c055fa8: [Microsoft.Android.Build] Bump to MSBuild 16.10.0 (#125)
  * dotnet/android-tools@49936d6: Merge pull request #124 from xamarin/update-libzipsharp
  * dotnet/android-tools@ef78dfc: Bump LibZipSharp to 2.0.0-alpha6
  * dotnet/android-tools@e3d708c: [BaseTasks] fix `\`-delimited paths on macOS (#122)
  * dotnet/android-tools@bdcf899: Reference the new Mono.Unix nuget (#123)
  * dotnet/android-tools@90d7621: [BaseTasks] add ABI detection for RIDs (#121)
  * dotnet/android-tools@79e3b97: [JdkInfo] handle invalid XML from /usr/libexec/java_home (#120)
  * dotnet/android-tools@81519fe: Add SECURITY.md (#119)
  * dotnet/android-tools@683f375: [Microsoft.Android.Build] Use MSBuild NuGets (#118)
  * dotnet/android-tools@2241489: [Xamarin.Android.Tools.AndroidSdk] Support Microsoft Dist JDK (#117)
  * dotnet/android-tools@52ef989: [Xamarin.Android.Tools.AndroidSdk] Fix CS0168 warning (#116)

Moving the Roslyn Analyzers initialization code from
`Directory.Build.props` to `Directory.Build.targets` (c936d09)
caused the Roslyn Analyzers to be applied to code imported from
`external/xamarin-android-tools`.

Previously the xamarin-android-tools code was unaffected because it
had its own `Directory.Build.props` file, so it did not traverse the
file system upwards any further to find the Java.Interop files.

Fixes for Roslyn issues has not been applied to `xamarin-android-tools`
code, so it adds ~150 warnings to the Java.Interop build.

dotnet/android-tools@ff73f925 added a
`Directory.Build.targets`, so updating `external/xamarin-android-tools`
to the latest xamarin-android-tools commit will prevent Java.Interop's
`Directory.Build.targets` file from being used, which will exclude
xamarin-android-tools from the Roslyn Analyzers.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 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.

3 participants