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

Prevent usage of 'in' with non-readonly structs by a compiler error #463

Merged
merged 1 commit into from Jul 28, 2019
Merged

Prevent usage of 'in' with non-readonly structs by a compiler error #463

merged 1 commit into from Jul 28, 2019

Conversation

dymanoid
Copy link
Contributor

Using a Roslyn Analyzer ReflectionIT.Analyzer.Structs, issue a compiler error when a non-readonly struct is passed to a method using the in modifier.

Passing a non-readonly struct with an in modifier can drastically decrease performance, because in the called method, the compiler will create defensive struct copies on every method call or property access of that passed struct.

Closes #440.

@krzychu124
Copy link
Member

Works only with VS 2019 because of missing dependency Microsoft.CodeAnalysis, Version=3.1.0.0 😕

Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

Builds for me.
As I am not using in keyword anyway, i'm happy to have this. It will be up to @krzychu124 to make it work on CI, or something? :)

@dymanoid
Copy link
Contributor Author

dymanoid commented Jul 26, 2019

Okay, didn't know you're on VS2017. Should I fork that analyzer and downgrade it back to VS2017 toolset, or you'd rather prefer to upgrade your IDE?

@krzychu124
Copy link
Member

I've upgraded my IDE 😃
I've had to change environment in AppVeyor to VS2019. Downside of that is longer queue for build 😕

@originalfoo
Copy link
Member

Updated dev tools wiki page to note VS 2019 requirement: https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/wiki/Dev-Tools

I assume it works on Rider too?

Will this also work on Visual Studio Code - I think @FireController1847 was using that for EVE dev.

@kvakvs
Copy link
Collaborator

kvakvs commented Jul 26, 2019

Clean and rebuild in both FullDebug and Release, worked on Rider with no visible effect.

@dymanoid
Copy link
Contributor Author

Maybe try to pass a non-readonly struct via in and see if that leads to a compile error?

@krzychu124
Copy link
Member

Works on Rider 2019.1.3 but only while building project/solution. I will check 2019,2 Early Access Preview

@dymanoid
Copy link
Contributor Author

On Rider, you need to explicitly enable Roslyn Analyzers somewhere in the settings. If I recall that correctly.

@originalfoo
Copy link
Member

Regarding activating the Roslyn Analyzers on Rider, post some details in a comment and I'll update wiki. I think the two Build guides will also need updating.

@dymanoid
Copy link
Contributor Author

https://www.jetbrains.com/help/rider/Settings_Roslyn_Analyzers.html

@krzychu124
Copy link
Member

krzychu124 commented Jul 26, 2019

I wanted to add screenshot but I couldn't upload due to timeout to GitHub Amazon S3 server 😄

In Rider 2019.2 EAP works too.

@originalfoo
Copy link
Member

Ah, the .dll has to be manually downloaded?

1>CSC : error CS0006: Metadata file '..\packages\ReflectionIT.Analyzer.Structs.0.1.0\analyzers\dotnet\cs\ReflectionIT.Analyzer.Structs.dll' could not be found

@dymanoid
Copy link
Contributor Author

No, the NuGet package manager should automatically restore the packages and download the binaries.

@originalfoo
Copy link
Member

originalfoo commented Jul 26, 2019

Looks like I built before it finished downloading lol. It's working now. I've not tested non-readonly structs and the in keyword yet.

EDIT: Could we add a unit test for testing that the analyser is working?

@dymanoid
Copy link
Contributor Author

This is not possible. The Analyzer will generate a compiler error, so the code won't compile and the unit test cannot be run. If the code compiles though and the test executes, that means the Analyzer doesn't work.

@krzychu124
Copy link
Member

@aubergine10 what do you think? Looks good to me.
Can we merge it?

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Note: I've not tested to see what happens when a non-readonly struct is used with in parameter (I don't really know how to as I've not worked with structs before). If that's not yet been tested merging would be premature.

@krzychu124
Copy link
Member

I did a few tests
It's easy, look 😉
Method:

private void Test(in Vector2 vec){
    Debug.Log(vec); // whatever
}

Call

Vector2 vec = new Vector2(1f, 1f);
Test(vec); //this line should create compilation error because Vector2 is not readonly struct

@originalfoo
Copy link
Member

👍

@originalfoo originalfoo added code cleanup Refactor code, remove old code, improve maintainability performance Make it faster! technical Tasks that need to be performed in order to improve quality and maintainability labels Jul 28, 2019
@originalfoo originalfoo added this to the 11.0 milestone Jul 28, 2019
@originalfoo originalfoo merged commit 3460aeb into CitiesSkylinesMods:master Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability performance Make it faster! technical Tasks that need to be performed in order to improve quality and maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn/error if in keyword used
4 participants