search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

Regression: XML documentation respects accessibility now? #77

Closed MichaelKetting closed 9 years ago

MichaelKetting commented 9 years ago

Original reported on [CodePlex](unter https://roslyn.codeplex.com/workitem/69) by jonskeet

C# 5 compiler version: 12.0.30110.0 Roslyn version: 0.6.40331.3

Sometimes it's useful for doc comments to be able to refer to a member which that piece of code wouldn't actually be able to access. For example, a bunch of private constants in one method might have to be kept in sync with a bunch of private constants in another. It's ugly, but it can happen. Within the same assembly, it feels reasonable - outside the same assembly, less so.

The C# 5 compiler allows this when checking method references in cref attributes; Roslyn doesn't.

Test file attached - under the C# 5 compiler it compiles with no warnings; under Roslyn I get:

Test.cs(7,31): warning CS1574: XML comment has cref attribute 'Ford.Method' that could not be resolved
Test.cs(20,31): warning CS1574: XML comment has cref attribute 'Zaphod.Method' that could not be resolved

(As context, I discovered this when trying to build Noda Time with Roslyn - we have warnings as errors, so this broke our build.)

Repro Sample on CodePlex: Test.cs

MichaelKetting commented 9 years ago

I just found this item. I also got a discussion for this here: https://roslyn.codeplex.com/discussions/547624 And the Connect issue https://connect.microsoft.com/VisualStudio/feedback/details/901988/cref-references-in-xml-doc-are-no-longer-able-to-link-to-non-public-members-from-a-different-type

I'm very interested in a fix for this. With our solution, I got dozens of errors from this and I would need to disable the warn-as-error for this accross several (10+ projects). That would mean I can't try Roslyn on the trunk. And that in turn makes it no fun to try Roslyn at all, give it performance spins etc...

paulomorgado commented 9 years ago

Not a solution, but because I don't like to ignore warnings, could you disable that warning project-wide?

MichaelKetting commented 9 years ago

Hi Paulo! I don't like to disable them, either. Please, see my remark above about why that isn't really a practical solution for me. Regards, Michael

sharwell commented 9 years ago

The C# compiler's processing for cref attributes involves translating them from a "C# reference" to their fully qualified form that appears in the final XML file. You should be able to work around this issue by explicitly using the fully qualified form as the cref attribute value.

For example:

/// Normal reference:
/// <see cref="X._privateField"/>
/// Explicit reference:
/// <see cref="F:Full.Namespace.Of.X._privateField"/>
MichaelKetting commented 9 years ago

Hi sharwell!

Thanks for bringing the fully qualified reference up. I only reported this workaround in my Connect issue but didn't add it to jonskeet's original CodePlex issue.

Yes, this would solve the reference problem, but it would cost me refactoring safety. R# can update references to members inside xml doc, but only if they are not using fully qualified references. I'm assuming, because the fully qualified form is no longer bound to a specific member within R#'s or Roslyn's code model. This is (I'm assuming) the reason why Roslyn no longer compains when you use the fully qualified form - it simply does not check if the specified member exists.

Therefor, it's not really an option for me to change all the usages to the fully qualified form (there's probably at least a hundred of them across the code base) unless I learn that this will not be fixed by Roslyn and a breaking change in the way the C# works will be made.

Best Regards, Michael

sharwell commented 9 years ago

@MichaelKetting Your comment is closely related to #45. I absolutely agree that accessibility should not be checked while resolving documentation comments, even across assemblies. However, it would probably make sense to continue filtering the IntelliSense lists to showing accessible members. Either that or use a rule like the following:

  1. If the documentation comment is on a publicly-accessible type or member (i.e. the type or member can be used from another assembly), then only include publicly-accessible types and members in the IntelliSense completion list.
  2. If the documentation comment is not on a publicly-accessible type or member, then show all items in the IntelliSense completion list.

This rule is based on the fact that projects which publish API documentation will generally filter the output to the exposed API surface. It would be undesirable for documentation associated with these items to include references to items which are not also included in that documentation.

Also note that features like QuickInfo, Go To Declaration, and Find All References would not filter documentation references, as that would completely defeat the purpose of allowing references.

For reference, I originally thought of the following more complicated set of rules, but decided the rules above are better overall.

The following represents my original thoughts, but is no longer my suggestion.

  1. If the referenced item is accessible in the scope of the documentation comment, show the item in IntelliSense.

  2. Otherwise, if the referenced item is in the same assembly (or perhaps the same solution) as the documentation comment and has accessibility greater than or equal to the scope of the documentation comment, show the item in the IntelliSense completion list.

    For example, if the documentation comment is on a public type or member and the target of the reference is internal or private, the member is not shown in IntelliSense. However, if the documentation comment is on an internal member and the reference is internal the item is shown in IntelliSense. The only way an out-of-scope private member shows in IntelliSense is when the documentation comment is on a private member.

  3. Otherwise, do not show the item in IntelliSense completion list (but do still show QuickInfo for the item).
amcasey commented 9 years ago

As of e1e49e9c77, crefs no longer respect accessibility. ☺

A little background: dev12 actually used a two-pass algorithm – the first pass respected accessibility and the second pass did not. This had the advantage that ambiguities would be resolved in favor or accessible symbols. It turns out that implementing this two-pass approach is much more difficult in Roslyn because of the new SemanticModel functionality (let me know if you want more details).

We started respecting accessibility because we found some corner cases where a cref might report an ambiguity because a source type/member and an internal one in another assembly. Clearly, this is undesirable, but, as it turns out, not as undesirable as being unable to reference your own inaccessible types/members. ☺

-Andrew From: Sam Harwell [mailto:notifications@github.com] Sent: Monday, January 26, 2015 7:02 AM To: dotnet/roslyn Subject: Re: [roslyn] Regression: XML documentation respects accessibility now? (#77)

@MichaelKettinghttps://github.com/MichaelKetting Your comment is closely related to #45https://github.com/dotnet/roslyn/issues/45. I absolutely agree that accessibility should not be checked while resolving documentation comments, even across assemblies. However, it would probably make sense to continue filtering the IntelliSense lists to showing accessible members. Either that or use a rule like the following:

  1. If the documentation comment is on a publicly-accessible type or member (i.e. the type or member can be used from another assembly), then only include publicly-accessible types and members in the IntelliSense completion list.
  2. If the documentation comment is not on a publicly-accessible type or member, then show all items in the IntelliSense completion list.

This rule is based on the fact that projects which publish API documentation will generally filter the output to the exposed API surface. It would be undesirable for documentation associated with these items to include references to items which are not also included in that documentation.

For reference, I originally thought of the following more complicated set of rules, but decided the rules above are better overall.

The following represents my original thoughts, but is no longer my suggestion.

  1. If the referenced item is accessible in the scope of the documentation comment, show the item in IntelliSense.
  2. Otherwise, if the referenced item is in the same assembly (or perhaps the same solution) as the documentation comment and has accessibility greater than or equal to the scope of the documentation comment, show the item in the IntelliSense completion list.

For example, if the documentation comment is on a public type or member and the target of the reference is internal or private, the member is not shown in IntelliSense. However, if the documentation comment is on an internal member and the reference is internal the item is shown in IntelliSense. The only way an out-of-scope private member shows in IntelliSense is when the documentation comment is on a private member.

  1. Otherwise, do not show the item in IntelliSense completion list (but do still show QuickInfo for the item).

— Reply to this email directly or view it on GitHubhttps://github.com/dotnet/roslyn/issues/77#issuecomment-71473403.

paulomorgado commented 9 years ago

Even cross-assemblies, @amcasey? If so, just on the same solution, right?

amcasey commented 9 years ago

The compiler has no concept of project or solution. There are a few places where we check whether something is coming from another Compilation, but this isn’t one of them. It should be the usual lookup rules (plus a few cref-specific quirks) without accessibility checks.

However, at a high level, you can think of it as being able to refer to any type/member in any assembly referenced by the containing project.

From: Paulo [mailto:notifications@github.com] Sent: Monday, January 26, 2015 2:44 PM To: dotnet/roslyn Cc: Andrew Casey (ROSLYN) Subject: Re: [roslyn] Regression: XML documentation respects accessibility now? (#77)

Even cross-assemblies, @amcaseyhttps://github.com/amcasey? If so, just on the same solution, right?

— Reply to this email directly or view it on GitHubhttps://github.com/dotnet/roslyn/issues/77#issuecomment-71554458.

paulomorgado commented 9 years ago

So, reflection might be involved regarding non public methods, right?

amcasey commented 9 years ago

I’m not sure I understand your question. Crefs don’t have a runtime component. The attribute value is bound to a symbol at compile-time and then transformed into another format to be written to the xml documentation file for the assembly.

From: Paulo [mailto:notifications@github.com] Sent: Monday, January 26, 2015 3:06 PM To: dotnet/roslyn Cc: Andrew Casey (ROSLYN) Subject: Re: [roslyn] Regression: XML documentation respects accessibility now? (#77)

So, reflection might be involved regarding non public methods, right?

— Reply to this email directly or view it on GitHubhttps://github.com/dotnet/roslyn/issues/77#issuecomment-71557869.

paulomorgado commented 9 years ago

I know crefs don't have a run-time component. I'm asking about compile-time.

Does the compiler use reflection to get information about external non public symbols. Is that any different from getting information about public symbols?

MichaelKetting commented 9 years ago

@sharwell I can see how #45 might end up requiring the same code paths but the underlying concepts are quite different, from a consumer's POV, as the method groups are a feature that wasn't previously supported and deals with referencing something that is not a member in the most direct sense of the word. To me, they're more related to referencing namespaces. Regardless of that, I'll happily take the method groups, too :)

As for Intellisense, I guess that starting to type the name and getting completion on that would be sufficient.

@amcasey That's great news, Andrew! Can't wait for the next VS 2015 drop!

How do you handle open issues? Should I close it based on this thread or would you close the issue based on your workflow?

Regards, Michael

amcasey commented 9 years ago

@paulomorgado Crefs are bound against the same symbols as source code. As during regular compilation, symbols from metadata are constructed based on output from a metadata reader. To the best of my knowledge, the compiler never inspects symbols using .NET reflection.

@MichaelKetting You're right, I should probably close this thread since the change has already been made. :)