Skip to content

Don't crash if you reference a generator built against .NET Framework#48400

Merged
Cosifne merged 7 commits intodotnet:release/dev16.8from
jasonmalinowski:fix-exception-for-adding-incompatible-source-generator
Oct 15, 2020
Merged

Don't crash if you reference a generator built against .NET Framework#48400
Cosifne merged 7 commits intodotnet:release/dev16.8from
jasonmalinowski:fix-exception-for-adding-incompatible-source-generator

Conversation

@jasonmalinowski
Copy link
Member

The compiler added a block to prevent loading generators that are built against the .NET Framework, since we know long term that we'll want them to be runnable on .NET Core. The IDE helper that deals with converting the error code to an IDE diagnostic needs to be updated to support this new error, or otherwise Visual Studio will crash.

Fixes #47845

The compiler added a block to prevent loading generators that are built
against the .NET Framework, since we know long term that we'll want them
to be runnable on .NET Core. The IDE helper that deals with converting
the error code to an IDE diagnostic needs to be updated to support this
new error, or otherwise Visual Studio will crash.

Fixes dotnet#47845
@jasonmalinowski jasonmalinowski force-pushed the fix-exception-for-adding-incompatible-source-generator branch from 4db95e4 to c271c72 Compare October 7, 2020 18:29
@jasonmalinowski jasonmalinowski self-assigned this Oct 7, 2020
@jasonmalinowski jasonmalinowski added this to the 16.8 milestone Oct 7, 2020
Comment on lines +6568 to +6569
<comment>{1} is the type that was loaded, {0} is the containing assembly.
</comment>
Copy link
Member Author

Choose a reason for hiding this comment

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

XML formatting got a bit screwed up here.

Copy link
Member

Choose a reason for hiding this comment

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

F'ing resource files are the bane of my developer existence. (If only there were a better way 👍 )

@chsienki chsienki force-pushed the fix-exception-for-adding-incompatible-source-generator branch from 1097171 to b8e7505 Compare October 8, 2020 20:19
@chsienki
Copy link
Member

chsienki commented Oct 8, 2020

Filled #48461 to track improving the generator load coverage on desktop.

@jasonmalinowski jasonmalinowski marked this pull request as ready for review October 9, 2020 17:59
@jasonmalinowski jasonmalinowski requested review from a team as code owners October 9, 2020 17:59
@jasonmalinowski
Copy link
Member Author

Tagging @dotnet/roslyn-compiler and @dotnet/roslyn-ide for reviews, since this is fixing issues on both sides.

}

[Fact]
// can't load a framework targeting generator, which these are in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

, which these are in desktop [](start = 53, length = 28)

I didn't really understand this part of the comment. Can it be removed? It seems like the previous statement is sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it just means that when running the desktop version of the test, the generators we reference are also desktop (because they're in the same assembly). I'll clean it up to make it more clear.

Copy link
Contributor

@cston cston left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM.

Copy link
Contributor

@tmeschter tmeschter left a comment

Choose a reason for hiding this comment

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

You're adding localizable resources after the cut-off for 16.8. You should get approval for a tenet exception before merging this in.

Get
Throw ExceptionUtilities.Unreachable
End Get
End Property
Copy link
Contributor

Choose a reason for hiding this comment

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

don't love that this is unreachable. can we still have some sort of test for this (so that if we do add SG support for VB, this fires for .net framework as well)?

Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

IDE changes LGTM

@Cosifne
Copy link
Member

Cosifne commented Oct 15, 2020

@jasonmalinowski
Is it ready to merge?

@Cosifne Cosifne merged commit 2674842 into dotnet:release/dev16.8 Oct 15, 2020
@jasonmalinowski jasonmalinowski deleted the fix-exception-for-adding-incompatible-source-generator branch October 15, 2020 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.