Bug 11876 - [CodeIssue] RedundantThisIssue have some missing cases
Summary: [CodeIssue] RedundantThisIssue have some missing cases
Status: RESOLVED FIXED
Alias: None
Product: Xamarin Studio
Classification: Desktop
Component: C# Binding ()
Version: Trunk
Hardware: PC Mac OS
: Normal enhancement
Target Milestone: ---
Assignee: Mike Krüger
URL:
Depends on:
Blocks:
 
Reported: 2013-04-21 09:19 UTC by Ji Kun
Modified: 2015-12-17 12:00 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Ji Kun 2013-04-21 09:19:17 UTC
It seems to me that this issue would give every possible redundant this qualifier a hint.

A simple missing case is as following:

class Foo
{
	private int Member;
	public Foo (int member)
	{
		this.Member = member;
	}
}

When running Unit Test, it fails to find issues.

After reading the code, I found a strange behavior.

var result = state.LookupSimpleNameOrTypeName(memberReference.MemberName, EmptyList<IType>.Instance, NameLookupMode.Expression);
bool isRedundant;
if (result is MemberResolveResult) {
		isRedundant = ((MemberResolveResult)result).Member.Region.Equals(member.Region);
} else if (result is MethodGroupResolveResult) {
isRedundant = ((MethodGroupResolveResult)result).Methods.Any(m => m.Region.Equals(member.Region));
} else {
return;
} 

The supposed to be MemberResolveResult case (e.g., the Foo class case) would be MethodGroupResolveResult in the end.

If we want to make this issue more aggressively, can we give hints to all the this qualifier except the ones hided by local var/parameters?
Comment 1 Mikayla Hutchinson [MSFT] 2013-04-22 10:38:37 UTC
The case of a parameter being assigned to a property of the same name but different case was deliberately excluded as this is often done for clarity. But maybe we could have a style policy for this.
Comment 2 Mike Krüger 2013-04-22 10:43:48 UTC
In fact the code issue has an option for turning this on or off. But the question to answer is:

where to place options for issues - and if we want to do it ... 

For this special case I could imagine a separate rule: redundant this in constructors. I don't really know what the best solution is - how does Resharper solve that ?

Do they have options/styles for code issues ?
If not, does their redundant this work in constructors ?

Inside constructors it's easier to read with this. - example:

this.param1 = param1;
this.Param2 = param2;
this.param3 = param3;

Is better than:

this.param1 = param1;
Param2 = param2;
this.param3 = param3;
Comment 3 Ciprian Khlud 2013-05-01 13:47:34 UTC
R# has the default naming policy for (private) fields is using camelCase + leading underscore: _<fieldName>, so 'this.' is really a clarity issue. For public/protected fields, it is using PascalCase.

So in this case, the code will be like:
this._param1 = param1;
this.Param2 = param2;
this._param3 = param3;

if you remove the redundant this:
_param1 = param1;
Param2 = param2;
_param3 = param3;

Which is clear that _param1 is a field, param1 is either a variable or a method's argument, Param2 is a property or public/protected field. So this. is not necessary and the code is also clear (based on the naming policy).
Comment 4 Mikayla Hutchinson [MSFT] 2013-05-01 13:48:41 UTC
But that's not *our* naming policy :)
Comment 5 Mike Krüger 2013-05-03 04:34:31 UTC
We'll need to think about this - it's not easy to decide what's the best approach is.
Comment 6 Mike Krüger 2015-11-23 03:41:41 UTC
Fixed in 6.0. MS made the decision for us - the name simplification removes always 'this.' if it's redundant.