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 InternalsVisibleTo to common types schema #6778

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

DamianEdwards
Copy link
Member

Fixes #6777

Since .NET 5, the <InternalsVisibleTo Include="MyProject.Assmebly" /> item type has been supported to generate the System.Runtime.CompilerServices.InternalsVisibleTo for the output assembly. The definition for this item type should be in the common types schema file so that editors (like Visual Studio) provide statement completion and a QuickInfo tooltip for it.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

LGTM. I was suspicious of the friend language, but it's used in the IVT docs.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

As rainersigwald suggested, "friend assembly" doesn't mean anything to me without looking it up. I'm wondering if there's a way we can explain the same concept briefly instead?

<xs:element name="InternalsVisibleTo" substitutionGroup="msb:Item">
<xs:annotation>
<xs:documentation>
<!-- _locID_text="InternalsVisibleTo" _locComment="" -->Specifies that types that are ordinarily visible only within the assembly are visible to the specified assemblies.
Copy link
Member

Choose a reason for hiding this comment

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

and methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought the exact same thing when I copied that text from the docs. It should likely be "types and members" to be even more correct. I'll give the wording a massage with the goal of just making it clearer here despite what the API docs have right now.

<xs:attribute name="Include" type="xs:string">
<xs:annotation>
<xs:documentation>
<!-- _locID_text="InternalsVisibleTo_Include" _locComment="" -->The name of the friend assembly to make internal types visible to, e.g. Microsoft.AspNetCore.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- _locID_text="InternalsVisibleTo_Include" _locComment="" -->The name of the friend assembly to make internal types visible to, e.g. Microsoft.AspNetCore.
<!-- _locID_text="InternalsVisibleTo_Include" _locComment="" -->The name of the friend assembly that gains visibility to internals of the other assembly.

?

Copy link
Member Author

Choose a reason for hiding this comment

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

The text is basically lifted exactly from the ref docs for the attribute. I think we should keep them the same so if we want to change it here we should change it there too.

InternalsVisibleToAttribute class description:
Specifies that types that are ordinarily visible only within the current assembly are visible to a specified assembly.

AssemblyName property description:
Gets the name of the friend assembly to which all types and type members that are marked with the internal keyword are to be made visible.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would prefer they both change. It had been very unclear, and it is in our style docs not to use things like e.g. because they aren't technically English.

If you want to leave it this way, though, I'm not blocking its insertion.

- Uses "internal types and members"
- Standardizes on "friend assembly" given that's the official term in docs already, inc. for InternalTypesVisibleToAttribute
- Removed "e.g." instance
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Aug 24, 2021
@DamianEdwards
Copy link
Member Author

Can we get this into .NET 6?

@Forgind
Copy link
Member

Forgind commented Aug 25, 2021

Should be able to. RPS has been flaky lately, but I'm rerunning it. I can prioritize this among the not-yet-merged PRs.

@Forgind Forgind merged commit 1630763 into main Aug 26, 2021
@RussKie RussKie deleted the damianedwards/internalsvisibleto branch September 16, 2021 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add InternalsVisibleTo item type to common types schema
3 participants