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

Implemented SA1121 #350

Merged
merged 4 commits into from
Jan 14, 2015
Merged

Implemented SA1121 #350

merged 4 commits into from
Jan 14, 2015

Conversation

pdelvo
Copy link
Member

@pdelvo pdelvo commented Jan 11, 2015

Fixes #54. Unfortunatly the diagnostic itself was already implemented in #306. But this one contains a bunch of unit tests so it might be a good idea to use them

@sharwell
Copy link
Member

This is currently blocked on #306. See #54 for additional information.

@pdelvo pdelvo mentioned this pull request Jan 11, 2015
@sharwell
Copy link
Member

Now that #306 is merged, can you rebase this and reduce it to a nice set of tests (plus enabling the diagnostic)? 👍

If you aren't comfortable with Git rebase, you can also do a pull from the master branch to bring in the updates.

@pdelvo
Copy link
Member Author

pdelvo commented Jan 11, 2015

This should bring in the tests. Im working on the code fix

{
static readonly Tuple<string, string>[] _referenceTypes = new Tuple<string, string>[]
{
new Tuple<string,string>("object", "Object"),
Copy link
Member

Choose a reason for hiding this comment

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

nameof(Object) for the right hand side here. 😉 The same for the items below.

@sharwell
Copy link
Member

Can you add this test?

namespace Foo
{
  using Int32 = System.UInt32;
  class Bar
  {
    Int32 value = 3;
  }
}

A diagnostic should be reported, but the target built-in type is actually uint due to the misleading using alias directive.

@sharwell
Copy link
Member

Also can you add this test?

namespace Foo
{
  using MyInt = System.Int32;
  class Bar
  {
    MyInt value = 3;
  }
}

@pdelvo
Copy link
Member Author

pdelvo commented Jan 11, 2015

What are the desired results here? Should using Int32 = System.UInt32; change to using Int32 = uint; or should the Int32 value = 3; change to uint value = 3; ? Or both? Currently it will report 2 diagnostics at MyInt value = 3; and the code fix im implementing is replacing it there

@sharwell
Copy link
Member

  1. Using alias directives are not allowed to use the language-specific keywords to refer to types. No diagnostic should be reported for the following line:

    using Int32 = System.Int32;
  2. In the first example I posted (the misleading using alias directive), the using alias directive would not be altered by the code fix. The reference to the alias would be changed from Int32 to uint.

  3. In the second example I posted, the using alias directive again would not change, and the reference to MyInt would be changed to int.

@pdelvo
Copy link
Member Author

pdelvo commented Jan 11, 2015

These new tests have some issues. The diagnostic analyzer is called twice for the same IdentifierToken. Do you know why this might happen?

new DiagnosticResultLocation("Test0.cs", 6, 5)
}
},
// Workaround because the diagnostic is called twice for the SyntaxNode
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 could not figure out why this happens. Maybe a roslyn bug?

@sharwell sharwell merged commit eedc55e into DotNetAnalyzers:master Jan 14, 2015
sharwell added a commit that referenced this pull request Jan 14, 2015
@sharwell
Copy link
Member

It's a bug somewhere in the unit test framework for this extension (modified from the one in the Roslyn templates). The diagnostics are only reported once for actual code, so I merged this and created an issue to find the bug in the unit test framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SA1121: UseBuiltInTypeAlias
2 participants