exercism/csharp-analyzer

An analyser for C#

Exercism C# analyzer

An analyzer can automatically detect issues with submissions and comment on them.

The C# analyzer implements the analyzer interface. It uses Roslyn to parse the submission's source code into syntax trees, which are then analyzed for known patterns.

Analyzing a solution

To analyze a solution, follow these steps:

1. Open a command prompt in the root directory.
2. Run ./analyze.ps1 <exercise> <directory>. This script will run the analyzer on the specified directory.
3. Once the script has completed, the analysis results will be written to <directory>/analysis.json.

Analyzing multiple solutions

To analyze multiple solutions at once, follow these steps:

1. Open a command prompt in the root directory.
2. Run ./bulk-analyze.ps1 <exercise> <directory>. This script will run the analyzer on each directory sub-directory of <directory>.
3. Once the script has completed, it will:
   1. Output general staticics to the console.
   2. Write detailed analysis results to <directory>/bulk_analysis.json.

Analyzing a solution using Docker

To analyze a solution using a Docker container, follow these steps:

1. Open a command prompt in the root directory.
2. Run ./run-in-docker.ps1 <exercise> <directory>. This script will:
   1. Build the analyzer Docker image (if necessary).
   2. Run the analyzer Docker image (as a container), passing the specified exercise and directory arguments.
3. Once the script has completed, the analysis result can be found at <directory>/analysis.json.

Note that the Docker image is built using the .NET IL Linker, which is why building can be quite slow.

Source code formatting

This repository uses the dotnet-format tool to format the source code. There are no custom rules; we just use the default formatting. You can format the code by running the ./format.ps1 command.

Scripts

The scripts in this repository are written in PowerShell. As PowerShell is cross-platform nowadays, you can also install it on Linux and macOS.

csharp-analyzer's Issues

Fix build

It looks like the build is actually failing, but it still is marked as being successful.

I think the issue might be with the dotnet test call in the test.ps1 file not returning the correct exit code. The fix might be something like this: https://github.com/exercism/csharp/blob/main/test.ps1#L120

Run CI tests using Docker image

This allows us to verify that the Docker image actually works

Add Docker support

We should use the Alpine-linux image

Check if Directory.Build.props file is still needed

Add feature for constants exercise

Task 5 of the constants exercise is to "Ensure that the developers cannot be tampered with". The intention here is to encourage the student to wrap the developers dictionary in an instance of ReadOnlyDictionary<string, Identity> in the Authenticator.GetDevelopers() method and to return that type or IReadOnlyDictionary<string, Identity> from the method.

This concept is expressed elsewhere as "preventing code circumventing the published or formal API of a class".

Improve matching of namespaces

There are two features that can help here:

• Removing the unused using statements
• Sorting the remaining (used) using statements

Add features for basics exercise

See the basics exercise.

There are two features to be added:

• Verify that the RemainingMinutesInOven() method calls the ExpectedMinutesInOven() method.
• Verify that the ElapsedTimeInMinutes() method calls the PreparationTimeInMinutes() method.

The bolierplate is here.
Example code is here.

Document bulk updater

Re-enable compilation

Re-enabling compilation will allow for various optimizations:

• Reducing syntax
• Running linters
• Be more precise when matching

Convert to prescribed Docker format

See https://github.com/exercism/automated-mentoring-support/blob/master/docs/docker.md

Enable travis build

@kytrinyx could you perhaps enable Travis builds for this repository?

Integrate dotnet format to format source code

Use files for analyzer tests

See https://github.com/exercism/automated-mentoring-support/issues/16

Remove in-memory compilation

The current analyzer does in-memory compilation, but that has some disadvantages:

1. The code is more complex.
2. There is a definite performance hit when compiling things.

What we gain from compiling the code, is:

1. The ability to verify that the source code actually compiles successfully (you can't verify this with syntax alone).
2. We can simplify things a bit using the Simplifier class.

After doing quite a bit of research, I’m farily disappointed in the C# compiler. There are many built-in fixes to cleanup code, but they are not available to me as a caller. With the recent release of the dotnet format tool,we might be able to use that as inspiration to format the code.

edit: dotnet format is still quite limited unfortunately, see https://github.com/dotnet/format/wiki/Supported-.editorconfig-options

Enable `nullable` compiler warnings

Enable the C# 8 nullable feature for the analyzer.

Add support for capitalization conventions

Any deviation from the official capitalization conventions should be marked as disapproved.

Add feature for method overloading exercise

See the method-overloading exercise.

The following feature should be added:

There are three overloaded Describe() methods which each take a single parameter (character, destination and travel method. They are not addressed by this issue.

In addition to the above 3 overloads the instructions require that the code should allow:

• the caller to specify all 3 aspects in one call (character + destination + travel method).

• or the caller to specify just 2 aspects, character and destination, in which case TravelMethod.Walking should be used as the travel method.

There are 2 ways for the student to approach this. Firstly, to include one overload with 3 parameters and a separate one with 2 parameters. In fact the boilerplate takes this approach.

public static string Describe(Character character, Destination destination, TravelMethod travelMethod) {}
public static string Describe(Character character, Destination destination) {}

The alternative is to omit the 2 parameter method altogether and give the final (TravelMethod) parameter of the 3 parameter method a default value of TravelMethod.Walking as is shown in the example code.

public static string Describe(Character character, Destination destination, TravelMethod travelMethod = TravelMethod.Walking) {}

The analyzer should report that the second approach, giving the parameter a default value, is preferable. This should also handle the case where named parameters are used.

Add features for properties exercise

See the properties exercise.

There 7 features required for the properties exercise:

1. If WeighingMachine.Units is not auto-implemented
   then the following comment should be made: "The appropriate form
   for a property such as WeighingMachine.Units which has no validation or other processing required is
   that for an auto-implemented property". - Approved with comment.

2. If WeighingMachine.DisplayWeight has a non-private set accessor
   then the following comment should be made: "It is not approprirate
   for a property such as WeighingMachine.DisplayWeight which simply returns a value
   to have a set accessor. That should be removed.". - Approved with comment.

3. If WeighingMachine.USDisplayWeight has a non-private set accessor
   then the following comment should be made: "It is not approprirate
   for a property such as USWeighingMachine.DisplayWeight which simply returns a value
   to have a set accessor. That should be removed.". - Approved with comment.

4. If USDisplayWeight.Pounds has a non-private set accessor
   then the following comment should be made: "It is not approprirate
   for a property such as USDisplayWeight.Pounds which simply returns a value
   to have a set accessor. That should be removed.". - Approved with comment.

5. If USDisplayWeight.Ounces has a non-private set accessor
   then the following comment should be made: "It is not approprirate
   for a property such as USDisplayWeight.Ounces which simply returns a value
   to have a set accessor. That should be removed.". - Approved with comment.

6. If WeighingMachine.TareAdjustement is not an auto-implemented property
   then the following commen should be made: "A succinct way of implementing
   WeighingMachine.TareAdjustment is as an auto-implemented property with a
   private get accessor". - Approved with comment.

7. If WeighingMachine.TareAdjustment is an auto-implemented property
   but the get accessor is non-private then the following comment should be made:
   "A non-private set accessor is not appropriate for WeighingMachine.TareAdjustment
   as the instructions stipulate that the value must not be available outside the
   class". - Disapproved.

See the boilerplate here.
See the completed example here.

Add features for flag-enums exercise

See the flag-enums exercise.

Two features are required for this exercise:

• Verify that the Permission enum is marked with the [Flags] attribute.
• Suggest using byte as the enum's backing type if no backing type was explicitly specified.

The boilerplate is here.
Example code is here.

Two-fer paramater field as default should not lead to a comment

A two fer solution that uses a field as default for the parameter leads to the csharp.two-fer.invalid_default_value comment. This should not happen.

See https://exercism.io/mentor/analyses/3746 as an example.

Convert returned comments to keys

See exercism/automated-analysis#11

The master branch will be renamed to main

In line with our new org-wide policy, the master branch of this repo will be renamed to main. All open PRs will be automatically repointed.

GitHub will show you a notification about this when you look at this repo after renaming:

In case it doesn't, this is the command it suggests:

git branch -m master main
git fetch origin
git branch -u origin/main main

You may like to update the primary branch on your forks too, which you can do under Settings->Branches and clicking the pencil icon on the right-hand-side under Default Branch:

We will post a comment below when this is done. We expect it to happen within the next 12 hours.

Add generic guidance for return-after-catch

Given the code

int Foo()
{
    try
   {
       // try stuff
   }
   catch (Exception)
   {
       // catch stuff
   }
   return 0;
}

Advise that the return statement should be placed in the try block.

The same applies to finally, mutatis mutandis.

Obviously the catch or finally block must not occur within some conditional control flow and must occur as the final statement in the method.

I assume we have a generic test to ensure that goto is prohibited!

Add features for inheritance exercise

See the inheritance exercise.

There are two features required:

• Verify that the constructor of the Character class uses the protected modifier.
• Verify that the various fields used (hit points, spell prepared and potion drunk) use the private modifier. i.e. all fields should be private

The boilerplate is here.

Allow bulk analysis of solutions

To verify how the analyzer performs, it is vital to test it against actual user solutions. These solutions should then be analyzed in bulk, and a report should be output stating the results:

• Total number of analyzed solutions
• Total number of solutions per returned status
• Percentage of solutions with each status
• Etc.

It should also be able to list the files in a specific category, which is particularly useful when verifying if valid solutions are not being approved.

Fix CI

Add CONTRIBUTING.md file

Move update expected files from test project to test.ps1

Similar to how the expected files are updated in the C# representer.

Convert string-based comparison to syntax-based comparison

Support simplification of syntax tree when comparing

By using Roslyn's Simplifier class, we can "reduce" a syntax tree, which:

1. Shortens dotted names to their minimally qualified form
2. Removes unnecessary parentheses
3. Removes unnecessary casts/conversions
4. Removes unnecessary escaping
5. Rewrites explicit calls to extension methods to use dot notation
6. Removes unnecessary tuple element names and anonymous type member names

This will help us match solutions that are slightly off.

Consider what to do with non-default formatting

Should we disapprove or comment on non-default formatting, like braces on the same line?

Multiple comments on approval/disapproval

Currently the analysers always approve or disapprove submission with one comment. I would like to start a discussion if it would be better to give multiple comments. This prevents users needing to make multiple submissions to get all the available feedback. This could tire or annoy users. It also gives more feedback to users that do not make a new submission for a approved exercise, but take the lessons learned to a new exercise.

I think it is important to not overload users with to many comments at one time, so maybe we should set a maximum amount or have different levels of comments and only return for the most severe level we detect comments for.

Would love to hear ideas about this.

Extra info:
It has been decided that this is a good idea. Current progress.

• Two-fer
• Leap
• Gigasecond

Add analyzer for weighing-machine

Relates to exercism/csharp#1449

P.S.: I'd like to try this