Skip to content

Fix Internal API usage of PlatformUtils #179

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.

Sign up for GitHub

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

Open
jwren wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Internal API usage of PlatformUtils #179

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions third_party/src/main/java/com/jetbrains/lang/dart/sdk/DartSdkLibUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import com.intellij.platform.workspace.jps.entities.*;
import com.intellij.util.ArrayUtil;
import com.intellij.util.ArrayUtilRt;
import com.intellij.util.PlatformUtils;

import com.intellij.util.SmartList;
import com.jetbrains.lang.dart.ide.index.DartLibraryIndex;
import org.jetbrains.annotations.NotNull;
Expand All @@ -45,7 +45,8 @@ public final class DartSdkLibUtil {
};

public static boolean isIdeWithMultipleModuleSupport() {
return PlatformUtils.isIntelliJ() || "AndroidStudio".equals(PlatformUtils.getPlatformPrefix());
String prefix = System.getProperty("idea.platform.prefix");
return prefix == null || prefix.startsWith("Idea") || "AndroidStudio".equals(prefix);
Copy link
Collaborator

@pq pq Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It sort of seems like we should return false if prefix is null. Or? (EDIT: see below.)

Also: are we confident enough in our testing that we'd catch it if this implementation detail changed or do we think an "idea.platform.prefix" system property is a durable promise?

In general, this is an interesting kind of fix and I wonder if we shouldn't add a comment noting that we've inlined internal implementation (over using an internal API). I guess it's situational but I would generally hope for something more future-safe. Could we ask the prompt to try harder?

Regarding my first comment, looking at the implementation of getPlatformPrefix, it does default to "idea" if the property is unset so I guess this is probably behavior-preserving but that seems kind of odd. Maybe even a bug in their implementation? If not a bug today, are we confident it will stay true in the future?

Copy link
Collaborator

@pq pq Jan 14, 2026

Choose a reason for hiding this comment

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

TL;DR: in general I'd minimally want a comment explaining where the magic is coming from; maximally, prefer a proper API and ask Gemini to try harder to find one

Copy link
Collaborator

@pq pq Jan 14, 2026

Choose a reason for hiding this comment

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

Looking closer at the API docs, it looks like there actually is a preferred way to handle this...

See:

https://github.com/JetBrains/intellij-community/blob/idea/253.29346.240/platform/core-api/src/com/intellij/util/PlatformUtils.java#L13-L28

A little onerous, but prescribed. Worth evaluating at least!

Copy link
Member Author

@jwren jwren Jan 15, 2026

Choose a reason for hiding this comment

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

I've thought a lot about this and finally came around to this implementation. If we were to get our verifications down to zero in all categories then with future changes we'd be safe-guarded against new entries in a way that we aren't today.

As for this change, I believe that we actually want to not need these kinds of checks at all, this change preserved the old functionality in a way that is more future proof which I thought was important.

Copy link
Collaborator

@pq pq Jan 15, 2026

Choose a reason for hiding this comment

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

If we were to get our verifications down to zero in all categories then with future changes we'd be safe-guarded against new entries in a way that we aren't today.

You mean because if we had zero violations we could have new failures break the build? I totally agree that we want to get there but I don't think we want to take on technical debt along the way. Or maybe I'm misunderstanding your point?

As for this change, I believe that we actually want to not need these kinds of checks at all, this change preserved the old functionality in a way that is more future proof which I thought was important.

How is it future-proof? The change proposes inlining an implementation that could change at any time. Or maybe I'm misunderstanding? Anyway, this may all be besides the point since the docs suggest there's actually a way to do this that only uses public API which should be really future-proof. Or maybe you evaluated and disqualified that approach?

}

public static void ensureDartSdkConfigured(final @NotNull Project project, final @NotNull String sdkHomePath) {
Expand Down
Loading