Skip to content

fix: resolve inheritance hiding and covariant array warnings#9046

Open
NickKhalow wants to merge 3 commits into
devfrom
chore/inheritance-and-covariance
Open

fix: resolve inheritance hiding and covariant array warnings#9046
NickKhalow wants to merge 3 commits into
devfrom
chore/inheritance-and-covariance

Conversation

@NickKhalow

Copy link
Copy Markdown
Contributor

Pull Request Description

What does this PR change?

Resolves two classes of type-safety compiler warnings flagged by static analysis: member-hiding (CS0108 / CS0114) and covariant array conversion (CoVariantArrayConversion). No behavioral change is intended — these are correctness/clarity fixes that make hiding explicit and remove an array-write hazard.

CoVariantArrayConversion — Derived[] exposed as IBase[]:
Exposing a Derived[] through an IBase[] surface allows an element write that throws ArrayTypeMismatchException at runtime. Removed the writable-array surface:

  • ICommunityMemberPagedResponse.members and both implementations (GetCommunityMembersResponse, GetCommunityInviteRequestResponse) now return IReadOnlyList<ICommunityMemberData> — a safe covariant, read-only view with no write path. The single consumer (CommunityRequestsReceivedGroupView.SetRequestReceivedMemberItems) was updated to match; all call sites only iterate.
  • ReleaseMemorySystemShould test — the array literal is now explicitly typed new UnloadStrategyBase[] { ... } so the array's runtime type matches the field.

CS0108 / CS0114 — member hides an inherited member:
For each warning, applied the appropriate fix:

  • new where the hiding is intentional API surface — chiefly the MapRenderer marker interfaces (ICategoryMarker, ISceneOfInterestMarker, ISearchResultMarker, IClusterMarker, IPinMarker, IAtlasController) re-declaring base-interface members, plus IEmote/ITrimmed* and a few fields/types.
  • Removed the redundant declaration where a member merely duplicated the inherited one (e.g. IMemoryProfiler.SystemUsedMemoryInBytes, redundant shadowing fields on the toggle context-menu settings).

Test Instructions

Pure warnings/type-safety cleanup with no intended behavioral change — primarily a sanity check that nothing regressed.

Steps (standard run):

metaforge explorer run XXXX  # ← replace with this PR number

Expected result:

  • App boots normally. Communities member/requests lists still populate, the map renderer markers (categories, points of interest, search results, clusters, pins) still render and animate, emote/wearable loading still works, the memory profiler still reports, and the context-menu toggles with icons still display correctly.

Test Steps

  1. Open Communities → a community card → Members and Requests tabs; verify lists populate.
  2. Open the map; verify category / POI / search-result markers appear, cluster, and animate on hover/select.
  3. Open a context menu that uses icon toggles (e.g. settings) and verify icons/colors render.
  4. Confirm emotes and wearables still load on the avatar.

Additional Testing Notes

  • Run the EditMode test suite — ReleaseMemorySystemShould was touched (array typing) and should still pass.
  • These are compile-time-only changes; if it compiles and the above areas behave as before, the change is verified.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

@NickKhalow
NickKhalow requested review from a team as code owners June 22, 2026 14:48
@github-actions

Copy link
Copy Markdown
Contributor

badge

New build in progress, come back later!

@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

PR Review in Progress

  • Gather context (reading CLAUDE.md, docs, PR diff)
  • Review changed files
  • Post findings

View job run

{
internal readonly Sprite toggleIcon;

public ToggleWithIconContextMenuControlSettings(Sprite toggleIcon, string toggleText, Action<bool> toggleAction, RectOffset horizontalLayoutPadding = null, int horizontalLayoutSpacing = 30,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compile error (CS0191): Removing internal readonly Sprite toggleIcon from this class causes this.toggleIcon = toggleIcon in the constructor body to target the inherited readonly field on ToggleContextMenuControlSettings. C# only allows readonly fields to be assigned in a constructor of the declaring class, so this emits CS0191.

The correct fix to suppress the CS0108 warning without breaking the build is:

Suggested change
public ToggleWithIconContextMenuControlSettings(Sprite toggleIcon, string toggleText, Action<bool> toggleAction, RectOffset horizontalLayoutPadding = null, int horizontalLayoutSpacing = 30,
internal new readonly Sprite toggleIcon;

Alternatively, change the base-constructor call to the overload that accepts toggleIcon and remove the body assignment:

: base(toggleText, toggleAction, toggleIcon, horizontalLayoutPadding, horizontalLayoutSpacing, horizontalLayoutReverseArrangement)

but that is a behavioural change (base-class field would become non-null) and is out of scope for a warning-cleanup PR.

@@ -8,7 +8,6 @@ public class ToggleWithIconAndCheckContextMenuControlSettings : ToggleContextMen
{
internal readonly ToggleGroup toggleGroup;
internal readonly Sprite icon;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compile error (CS0191): Same issue as ToggleWithIconContextMenuControlSettings. Removing internal readonly Color iconColor here means line 28 (this.iconColor = iconColor == …) targets the inherited readonly field on ToggleContextMenuControlSettings, which C# does not permit from a derived-class constructor.

Suggested change
internal readonly Sprite icon;
internal readonly ToggleGroup toggleGroup;
internal readonly Sprite icon;
internal new readonly Color iconColor;

@claude

This comment has been minimized.

TrimmedWearableDTO TrimmedDTO { get; }


public bool IsSmart() =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compile error (CS1061): Removing TrimmedWearableDTO TrimmedDTO { get; } means the default method IsSmart() now resolves TrimmedDTO to the base ITrimmedAvatarAttachment.TrimmedDTO, which has return type TrimmedAvatarAttachmentDTO. That abstract base class has no metadata field (only the generic subclass TrimmedAvatarAttachmentDTO<TMetadata> does), so TrimmedDTO.metadata.isSmart fails with CS1061.

The fix that's consistent with how ITrimmedEmote was corrected is to keep the re-declaration but mark it explicit with new:

Suggested change
public bool IsSmart() =>
public bool IsSmart() =>

Apply this suggestion only after restoring the declaration above it:

new TrimmedWearableDTO TrimmedDTO { get; }

i.e., the full corrected block:

public interface ITrimmedWearable : ITrimmedAvatarAttachment<TrimmedWearableDTO>
{
    public int Amount { get; set; }
    bool IsCompatibleWithBodyShape(string bodyShape);

    new TrimmedWearableDTO TrimmedDTO { get; }

    public bool IsSmart() =>
        TrimmedDTO.metadata.isSmart;
    ...
}

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @NickKhalow's task in 4m 8s —— View job


PR Re-Review — fix: resolve inheritance hiding and covariant array warnings

  • Gather context (reading CLAUDE.md, docs, PR diff)
  • Check if previous blocking issues were resolved — all 3 are still present
  • Implement fixes locally (push blocked by repo permissions — see diffs below)

Status

The PR was synchronised but the three compile errors flagged in the previous review are unchanged. Exact fixes are below — all were verified locally.


Blocking Issues (still present)

1. ToggleWithIconContextMenuControlSettings.cs — CS0191

The derived-class field was removed but the constructor still assigns this.toggleIcon = toggleIcon to the inherited internal readonly field. C# only allows readonly fields to be assigned in a constructor of the declaring class.

The base class already has a constructor overload that accepts Sprite toggleIcon — just call it:

// BEFORE (broken — CS0191)
public ToggleWithIconContextMenuControlSettings(Sprite toggleIcon, string toggleText, Action<bool> toggleAction, ...)
    : base(toggleText, toggleAction, horizontalLayoutPadding, horizontalLayoutSpacing, horizontalLayoutReverseArrangement)
{
    this.toggleIcon = toggleIcon;
}

// AFTER (fix — delegate to the Sprite-accepting base overload)
public ToggleWithIconContextMenuControlSettings(Sprite toggleIcon, string toggleText, Action<bool> toggleAction, ...)
    : base(toggleText, toggleAction, toggleIcon, horizontalLayoutPadding, horizontalLayoutSpacing, horizontalLayoutReverseArrangement) { }

2. ToggleWithIconAndCheckContextMenuControlSettings.cs — CS0191

Same pattern: internal readonly Color iconColor was removed, but this.iconColor = iconColor == default ? Color.white : iconColor in the derived constructor still targets the inherited readonly field. The 7-argument base-class overload already handles the default → Color.white logic:

// BEFORE (broken — CS0191)
: base(toggleText, toggleAction, null, horizontalLayoutPadding, horizontalLayoutSpacing, horizontalLayoutReverseArrangement)
{
    this.toggleGroup = toggleGroup;
    this.icon = icon;
    this.iconColor = iconColor == default ? Color.white : iconColor;
}

// AFTER (fix — pass iconColor to base)
: base(toggleText, toggleAction, null, horizontalLayoutPadding, horizontalLayoutSpacing, horizontalLayoutReverseArrangement, iconColor)
{
    this.toggleGroup = toggleGroup;
    this.icon = icon;
}

3. ITrimmedWearable.cs — CS1061

TrimmedWearableDTO TrimmedDTO { get; } was removed rather than annotated with new. The default method IsSmart() calls TrimmedDTO.metadata.isSmart. Without the re-declaration, TrimmedDTO resolves to ITrimmedAvatarAttachment.TrimmedDTO whose return type is the non-generic TrimmedAvatarAttachmentDTO abstract class — which has no metadata field (only the generic subclass TrimmedAvatarAttachmentDTO<TMetadata> does). Consistent with how ITrimmedEmote was correctly fixed:

// AFTER (fix — restore with `new` to suppress CS0108 cleanly)
bool IsCompatibleWithBodyShape(string bodyShape);

new TrimmedWearableDTO TrimmedDTO { get; }

public bool IsSmart() =>
    TrimmedDTO.metadata.isSmart;

Non-blocking (carry-over from previous review)

  • ITrimmedEmoteStorage / ITrimmedWearableStorage: the <param name="frameTimeBudget"> XML doc tag now sits on a type declaration rather than a method — it will produce a spurious CS1572 if XML doc output is enabled. Remove or rewrite it to describe the interface.

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches shared interfaces across avatar rendering, map renderer, UI, communities, and memory profiling subsystems.
QA_REQUIRED: YES

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant