Skip to content
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.

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

Add analyzer for platform-specific APIs #110

Merged
merged 15 commits into from
Jul 17, 2020

Conversation

terrajobst
Copy link
Contributor

@terrajobst terrajobst commented Apr 14, 2020

This provides the attribute and analyzer needed to realize #97.

accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved

namespace System.Runtime.Versioning
{
public abstract class OSPlatformAttribute : Attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Will these attributes exist in reference assemblies only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should spec that indeed. What does Xamarin do today? Generally speaking, we don't strip most of the attributes. Leaving them in allows runtime light-up, but I can see them causing bloat.

Copy link
Member

Choose a reason for hiding this comment

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

Xamarin doesn't have reference assemblies, there's a single implementation assembly which contains all the availability attributes. They're all removed by the managed linker at build time (if the linker is enabled).

@marek-safar
Copy link
Contributor

/cc @rolfbjarne

@rolfbjarne
Copy link
Member

/cc @spouliot

accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
@mhutch mhutch changed the title Add analzyer for platform-specific APIs Add analyzer for platform-specific APIs Apr 14, 2020
@timheuer
Copy link
Member

My general feedback here is that historically the trend to look for OS/runtime-specific versioning has been frowned upon. Even in the web world looking for if (IE7) {} kind of things has been a frowned upon approach in favor of looking at capabilities e.g., if (browserCanUseLocation()) {} -- here we are implementing a recommended pattern of explicit OS version checks (and how deep does it go? into patch release) versus a capability example. Is there a different design that would enable the analyzer to fire and say: if (Platform.CanCreateDirectorySecurity()) {} approach?

@terrajobst
Copy link
Contributor Author

terrajobst commented Apr 15, 2020

@timheuer

My general feedback here is that historically the trend to look for OS/runtime-specific versioning has been frowned upon. Even in the web world looking for if (IE7) {} kind of things has been a frowned upon approach in favor of looking at capabilities e.g., if (browserCanUseLocation()) {} -- here we are implementing a recommended pattern of explicit OS version checks (and how deep does it go? into patch release) versus a capability example. Is there a different design that would enable the analyzer to fire and say: if (Platform.CanCreateDirectorySecurity()) {} approach?

The reason it's frowned upon in web is because none of the features are browser-specific. However, there is such as thing as OS specific APIs and APIs that are introduced in a specific version. And given an analyzer we can tell people what version to check for. Capability APIs aren't as easy to tool as it's hard to correlate them back to specific APIs that need the check (see #111 if you're curious).

@mhutch
Copy link
Contributor

mhutch commented Apr 15, 2020

My general feedback here is that historically the trend to look for OS/runtime-specific versioning has been frowned upon. Even in the web world looking for if (IE7) {} kind of things has been a frowned upon approach in favor of looking at capabilities e.g., if (browserCanUseLocation()) {} -- here we are implementing a recommended pattern of explicit OS version checks (and how deep does it go? into patch release) versus a capability example. Is there a different design that would enable the analyzer to fire and say: if (Platform.CanCreateDirectorySecurity()) {} approach?

Both approaches are valid and complementary - see #97 (comment)

@jzabroski
Copy link

jzabroski commented May 7, 2020

The reason it's frowned upon in web is because none of the features are browser-specific. However, there is such as thing as OS specific APIs and APIs that are introduced in a specific version. And given an analyzer we can tell people what version to check for. Capability APIs aren't as easy to tool as it's hard to correlate them back to specific APIs that need the check (see #111 if you're curious).

⚠️ Brain dump
@terrajobst But to further @timheuer point: These kinds of arguments were made by jQuery development team, too. A loose cannon by the name of David Mark would debunk that, and explain how through careful API design, one could avoid "Browser-specific APIs". I used to eat popcorn and watch him flame jQuery and Sencha developers on Usenet on my lunch break as a junior developer. Rude guy, but had good points after you plucked through the ridiculous insults. Through picking the kernels out of my teeth, I learned a lot from him about not seeing things as platform-specific but as capability-specific. One BIG lesson I learned from David is it needs to be clear, transitively, that your API depends on either OS-detection or Capability-detection. And with Roslyn, why can't we automate a bunch of that information with simple annotations? So, let's say for example you're running tests on alpine linux which doesnt ship locales by default and you try a StringComparison enum value that doesn't make sense on alpine zero locale instance.

Frankly, a lot of the differences for most of us application developers are not even OS-specific. For UNIX, its ulibc vs glibc vs musl libc and link-load DWARF&ELF issues.

So, while on one hand, you do have a point, it doesn't mitigate the general preference to try to provide metadata around what that OS-specific capability really means. For example, POSIX gaurantees atomic file replacement, whereas Windows Replace Win32 call states nothing of the sort, except for an obscure Microsoft Research paper claiming it is atomic, and so its ambiguous as to whether File.Replace C# .NET call is atomic, or simply best-efforts atomic, and what OSes it can be atomic on, and how one can fallback to behaviors that may or may not be pseudo-atomic. You can easily see that PlatformAbstractions namespace should strive to advertise such ACID capabilities and/or design notes.

On the far end of the spectrum, you can dive into the docker file system abstraction logic and see all the ridiculous driver logic they created to support layers and think, "UGH, I give up - I need an docker-specific set of rules here." And to some extent, maybe only a certain subset of engineers should need to understand all the different failures that can happen with a file system and accept and adapt to rules for creating stackable filesystems. (I mean, how do you define a "capability" for some of the minimum viable product hacks they put into their driver code?)

accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
accepted/2020/platform-checks/platform-checks.md Outdated Show resolved Hide resolved
@buyaa-n buyaa-n requested a review from analogrelay June 2, 2020 06:41
mavasani added a commit to mavasani/roslyn-analyzers that referenced this pull request Jun 16, 2020
…low analysis.

Our current DFA framework supports tracking and analyzing dataflow states for entities and/or locations on different control flow paths. However, quite a few analyses do not require tracking such fine grained state, they only require tracking a CFG wide state which changes based on different operations encountered in the CFG. For example, platform checker analyzer described in dotnet/designs#110 requires the analysis to track which OS platforms and versions are valid on any given control flow path.

This PR adds an initial DFA for such a CFG wide state based analysis, which has a dummy analyzer (not exported) to validate the DFA. There are large number of follow-up items:
1. Replace the dummy analyzer with the real platform checker analyzer
2. Add extensive analyzer tests
3. Naming for added DFA: I have named it as "FlightEnabledAnalysis" based on the term that was used in the original feature request. I plan to rename the analysis and associated types to something more generic in a follow-up change
4. Refactor the DFA to extract out common logic with extensibility points so that more such analyses can be trivially written on top of it.
5. Add comments and doc comments
@terrajobst
Copy link
Contributor Author

I've update the document to match what we agreed on in the API review.

@terrajobst terrajobst merged commit 3b80dec into dotnet:master Jul 17, 2020
@terrajobst terrajobst deleted the plaform-checks branch July 17, 2020 23:47
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.