Skip to content

Conversation

matthewnitschke-wk
Copy link
Contributor

@matthewnitschke-wk matthewnitschke-wk commented Apr 13, 2023

FEA-1741

Issue Status

There has been a longstanding issue with scip-dart where fields are not correctly indexed due to the synthetic get/set elements which are backing every field

This pr takes into account this edge case, pulling from the examples in deprecatalog and lsif_indexer

@rmconsole-wf
Copy link

rmconsole-wf commented Apr 13, 2023

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

General Information

Ticket(s):

Code Review(s): #32
Release Image Tags:

  • drydock.workiva.net/workiva/scip-dart:4462922
  • drydock.workiva.net/workiva/scip-dart:pr-32

Reviewers: matthewnitschke-wk, kimlarson-wk

Additional Information

Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Version: scip-dart 1.0.6
	Release Ticket(s): RM-187414


Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Monday, April 17 02:04 PM CST

@aviary2-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@matthewnitschke-wk matthewnitschke-wk changed the title index fields Fix broken indexing on private fields Apr 13, 2023
@matthewnitschke-wk matthewnitschke-wk changed the title Fix broken indexing on private fields Correctly index fields Apr 13, 2023
@matthewnitschke-wk matthewnitschke-wk marked this pull request as ready for review April 13, 2023 22:34
@bender-wk bender-wk changed the title Correctly index fields FEA-1741: Correctly index fields Apr 13, 2023
@matthewnitschke-wk matthewnitschke-wk requested a review from a team April 13, 2023 22:35
Comment on lines +96 to +100
// When the identifier is a field, the analyzer creates synthetic getters/
// setters for it. We need to get the backing field.
if (element?.isSynthetic == true && element is PropertyAccessorElement) {
element = element.variable;
}

Choose a reason for hiding this comment

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

#nit should we go ahead and make this an else if to avoid the unnecessary second check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, its necessary to hit both

the CompoundAssignmentExpression is to get the correct ast node, it can assign the element to be PropertyAccessorElement

And if element is PropertyAccessorElement in any case, we want its variable, not it's element node

Its a bit strange, but we're bound to the criteria of the ast/static analysis by a lot of this stuff

@matthewnitschke-wk
Copy link
Contributor Author

QA +1

Verified generated scip files are correct, and no regressions exist in snapshots

@kimlarson-wk
Copy link

@Workiva/release-management-pp ready for merge.

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

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

Successfully merging this pull request may close these issues.

5 participants