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

FEA-3206: Fix NormalFormalParameters #111

Merged
merged 7 commits into from
Feb 1, 2024
Merged

Conversation

matthewnitschke-wk
Copy link
Contributor

@matthewnitschke-wk matthewnitschke-wk commented Jan 21, 2024

FEA-3206

Issue Status

Closes #110

Previously constructor parameters would incorrectly use this as the symbol for the reference. This pr addresses that problem

There was another problem where parameters would also, always declare a "definition". Parameters that are referencing an instance variable (Foo(this.bar)), are not a definition, just a reference. This pr also fixes that problem by not marking these as definitions

@aviary-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.

@@ -35,6 +35,7 @@
class Animal with SleepMixin {
// ^^^^^^ definition scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#
// documentation ```dart
// relationship scip-dart pub dart_test 1.0.0 lib/more.dart/SleepMixin# implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're running relationship indexing on basic-project now, snapshots were not previously generated due to failing CI that neglected to run on previous PRs

Comment on lines 56 to 58
// ^^^^ reference scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#name.
// ^^^^ definition local 0
// documentation ```dart
// ^^^^ reference scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#type.
// ^^^^ definition scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#<constructor>().(type)
// documentation ```dart
// ^^^^ reference scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#name.
// ^^^^ reference scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#type.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Few changes here:

  • This is the actual fix that the PR was doing, no longer referring to this as the declaration

    Animal(this.name, {required this.type}) {
    //     ^^^^ reference scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#name.

    becomes

    Animal(this.name, {required this.type}) {
    //          ^^^^ reference scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#name.
  • ^^^^ definition local 0 on name was removed - name is not actually a definition, its simply a reference to the name var on the class

  • definition scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#<constructor>().(type) was removed. Similar to name, there is not actual "definition" here, its just a reference

@@ -102,6 +99,7 @@
// ^^^^^^ reference scip-dart pub dart:core 2.19.0 dart:core/string.dart/String#
// ^^^^^^^^ definition scip-dart pub dart_test 1.0.0 lib/more.dart/Animal#toString().
// documentation ```dart
// relationship scip-dart pub dart:core 2.19.0 dart:core/object.dart/Object#toString(). implementation reference
Copy link
Contributor Author

Choose a reason for hiding this comment

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

String toString

is overriding here, relationship is accurately noting this

@@ -114,17 +112,17 @@
// documentation ```dart
// ^^^^ reference scip-dart pub dart:core 2.19.0 dart:core/list.dart/List#
// ^^^ reference scip-dart pub dart:core 2.19.0 dart:core/int.dart/int#
// ^^^^^^^ definition local 1
// ^^^^^^^ definition local 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the local variable changes here are just due to no longer declaring local 0 above, no cause for concern

// ^^^^ reference local 0
// ^^^^ definition local 1
// documentation ```dart
// ^^^^ reference local 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were previously always calculating a definition for parameters, even formal ones referring to instance variables, now we aren't, hence the removal of definition local 1

_far is a private instance variable, so the reference is just a local 0

Comment on lines 2 to 3
// definition scip-dart pub dart_test 1.0.0 lib/relationships.dart/
// ^^^^^^ definition scip-dart pub dart_test 1.0.0 lib/relationships.dart/Mammal#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the file was moved/renamed, these changes are just updating for that difference

@bender-wk bender-wk changed the title Fix formal parameters FEA-3206: Fix formal parameters Jan 21, 2024
@matthewnitschke-wk matthewnitschke-wk changed the title FEA-3206: Fix formal parameters Fix NormalFormalParameters Jan 21, 2024
@matthewnitschke-wk matthewnitschke-wk changed the title Fix NormalFormalParameters FEA-3206- Fix NormalFormalParameters Jan 21, 2024
@matthewnitschke-wk matthewnitschke-wk changed the title FEA-3206- Fix NormalFormalParameters FEA-3206: Fix NormalFormalParameters Jan 21, 2024
@matthewnitschke-wk matthewnitschke-wk marked this pull request as ready for review January 21, 2024 22:01
todbachman-wf
todbachman-wf previously approved these changes Jan 30, 2024
@matthewnitschke-wk
Copy link
Contributor Author

QA +1

  • CI does indeed pass

🚀 @Workiva/release-management-p 🚢

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

@rmconsole6-wk rmconsole6-wk merged commit 317211f into master Feb 1, 2024
12 checks passed
@rmconsole6-wk rmconsole6-wk deleted the fix_formal_parameters branch February 1, 2024 16:46
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.

Incorrect indexing referencing this.x in constructor
5 participants