[Upstream] Remove assumptions on super's description

[Saadnajmi]

Summary

This is a change upstreamed from our fork, React Native macOS: microsoft#1088

Original description:

We hit some crashes where replacing the chars in the string in the description was happening at an invalid range. That caused investigation into what these description methods are doing. We shouldn't have code assuming super's description will return in a certain format. Instead, just append any additional info we want to add to the end of the description.

Changelog

[IOS] [CHANGED] -Remove assumptions on super's description

Test Plan

CI should pass

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,508,872	-2,003
android	hermes	armeabi-v7a	7,822,848	-3,419
android	hermes	x86	8,986,447	-3,149
android	hermes	x86_64	8,842,620	-3,380
android	jsc	arm64-v8a	9,135,684	-2,189
android	jsc	armeabi-v7a	8,325,234	-3,601
android	jsc	x86	9,188,637	-3,326
android	jsc	x86_64	9,447,629	-3,546

Base commit: 96659f8
Branch: main

[javache]

Can you provide more details on the crash you've been seeing? Agree that you usually don't need this in production, but when they do show up in exception messages or log messages from internal bug reports we've gotten a lot of value out of them.

[Saadnajmi]

Can you provide more details on the crash you've been seeing? Agree that you usually don't need this in production, but when they do show up in exception messages or log messages from internal bug reports we've gotten a lot of value out of them.

The crash was macOS specific (the format of description differs slightly from iOS, so one of them caused a crash). I'll dig up the into + the other fix we had.

[Saadnajmi]

@javache The other half of the "fix" we made is here: microsoft#1088

These descriptions were made by replacing a character in the existing description that existed on iOS, but not on macOS. That caused a crash for us. We fixed this by instead just appending to the super class's description, rather than making assumptions about its structure and modifying it.

If you still like this change, I can roll that refactor into this PR as well. Or I can make a change with just the refactor,but not limiting description to debug if that works better internally. I know you mentioned the description is useful internally (I'm not sure if in debug or release) so I'd understand if you'd rather not take either change altogether.

[javache]

Yes, the replacing of super's description is purely cosmetic, so the changes in https://github.com/microsoft/react-native-macos/pull/1088/files would make sense to pull in upstream.

My preference would be to not add the #if RCT_DEBUG guards here and just port over the fix. If there's a desire to have release builds without these DEBUG methods, we should look at a different compiler flag to optionally enable that.

[Saadnajmi]

Yes, the replacing of super's description is purely cosmetic, so the changes in https://github.com/microsoft/react-native-macos/pull/1088/files would make sense to pull in upstream.

My preference would be to not add the #if RCT_DEBUG guards here and just port over the fix. If there's a desire to have release builds without these DEBUG methods, we should look at a different compiler flag to optionally enable that.

We actually just use #if DEBUG in our fork. I changed it to RCT_DEBUG things that was more appropriate. Would you prefer that? Or other something like if #HAS_DESCRIPTIONS ?

[Saadnajmi]

@javache Looking into it, I think you're right that description shouldn't be debug only. Otherwise, this wouldn't exist :D
https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418703-debugdescription?language=objc

I updated this PR to upstream the refactor, and not the DEBUG ifdefs.

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@javache merged this pull request in a5bc6f0.