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

Distinguish between get/set property #39

Closed
pjanotti opened this Issue Sep 25, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@pjanotti
Contributor

pjanotti commented Sep 25, 2017

This can be useful to more precise diagnostic messages, need to evaluate if there are downsides (e.g.: size of resource?)

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Sep 27, 2017

Member

Agreed. The way I'd do this is record either the setter, the getter, or the property (if both getter and setter are affected) in the CSV files.

In the lookup, I'd handle it as follows:

  • AssignmentExpression to check for the setter and disable walking the target to avoid also looking for the getter/property
  • Change the generic handler to also check for the property getter when the symbol is a property

We might want to do the same for events, but I don't think adders/removers will likely have different data.

Member

terrajobst commented Sep 27, 2017

Agreed. The way I'd do this is record either the setter, the getter, or the property (if both getter and setter are affected) in the CSV files.

In the lookup, I'd handle it as follows:

  • AssignmentExpression to check for the setter and disable walking the target to avoid also looking for the getter/property
  • Change the generic handler to also check for the property getter when the symbol is a property

We might want to do the same for events, but I don't think adders/removers will likely have different data.

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Nov 29, 2017

Member

FYI: This bug is causing false positives for the demo we've been using for a while, specifically Console.WindowWidth and Console.WindowHeight. The setter isn't supported, but the getter is.

https://github.com/dotnet/corefx/blob/93764c0991d167e3b74f41f1ea129004ce10598c/src/System.Console/src/System/ConsolePal.Unix.cs#L253-L263

Member

terrajobst commented Nov 29, 2017

FYI: This bug is causing false positives for the demo we've been using for a while, specifically Console.WindowWidth and Console.WindowHeight. The setter isn't supported, but the getter is.

https://github.com/dotnet/corefx/blob/93764c0991d167e3b74f41f1ea129004ce10598c/src/System.Console/src/System/ConsolePal.Unix.cs#L253-L263

@OliaG OliaG added this to the 0.2.0-alpha milestone Dec 1, 2017

@pjanotti

This comment has been minimized.

Show comment
Hide comment
@pjanotti

pjanotti Dec 5, 2017

Contributor

First pass by simply breaking down get/set gets an increase of ~13KB on the csv loaded as resource. I didn't measure the impact on the DB in memory yet but likely we will be good on this regard.

Regarding the events there is only 1 event was detected by the tool, and as expected it has both add and remove throwing PNSE. At such low cost I am tempted to leave it and deal it in a manner symmetrical to the properties.

Contributor

pjanotti commented Dec 5, 2017

First pass by simply breaking down get/set gets an increase of ~13KB on the csv loaded as resource. I didn't measure the impact on the DB in memory yet but likely we will be good on this regard.

Regarding the events there is only 1 event was detected by the tool, and as expected it has both add and remove throwing PNSE. At such low cost I am tempted to leave it and deal it in a manner symmetrical to the properties.

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Dec 6, 2017

Member

First pass by simply breaking down get/set gets an increase of ~13KB on the csv loaded as resource. I didn't measure the impact on the DB in memory yet but likely we will be good on this regard.

Is that because we record the data twice or because the signatures are longer? We can fix the former by continue to recording a single row for the property (if both setter and getter throw).

Member

terrajobst commented Dec 6, 2017

First pass by simply breaking down get/set gets an increase of ~13KB on the csv loaded as resource. I didn't measure the impact on the DB in memory yet but likely we will be good on this regard.

Is that because we record the data twice or because the signatures are longer? We can fix the former by continue to recording a single row for the property (if both setter and getter throw).

@pjanotti

This comment has been minimized.

Show comment
Hide comment
@pjanotti

pjanotti Dec 6, 2017

Contributor

Mostly it seems due to longer signatures: most of the properties are replaced by a single get or set that is a bit longer, few like registry then two lines where before we had just one.

Contributor

pjanotti commented Dec 6, 2017

Mostly it seems due to longer signatures: most of the properties are replaced by a single get or set that is a bit longer, few like registry then two lines where before we had just one.

pjanotti added a commit to pjanotti/platform-compat that referenced this issue Dec 6, 2017

WIP: Breaking properties into get and set
Many of the false positives are associated with properties since the
analyzer is not distinguishing between the get and set of properties and
many times the only one of the set or get throws PNSE.

On this initial commit breaking down events but likely will remove it,
for now I want to see if there is any event that does not throw for
both.

Fixes dotnet#39

@pjanotti pjanotti closed this in #74 Dec 15, 2017

pjanotti added a commit that referenced this issue Dec 15, 2017

Breaking properties into get and set (#74)
* WIP: Breaking properties into get and set

Many of the false positives are associated with properties since the
analyzer is not distinguishing between the get and set of properties and
many times the only one of the set or get throws PNSE.

On this initial commit breaking down events but likely will remove it,
for now I want to see if there is any event that does not throw for
both.

Fixes #39

* 2nd part for separating get/set prop

Changes to analyzers, tests, and exception files.

* Fix merge issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment