chore: remove more unused code

[incendial]

After the mention here I ran the unused check again with a different flag and got a bunch of new highlights. Some of them look illegal 😅.

Note: not all of the removed code might be unused intentionally, but I have no context of that. Also, there are might be not obvious to me false-positives.

RELEASE_NOTE_EXCEPTION=[Internal refactoring]

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read the [Flutter Style Guide] recently, and have followed its advice.
• I signed the [CLA].
• [-] I listed at least one issue that this PR fixes in the description above.
• [-] I updated/added relevant documentation (doc comments with ///).
• [-] I added new tests to check the change I am making, or there is a reason for not adding tests.

[incendial]

@jacob314 @polina-c fyi

[incendial]

Hm, I didn't notice that devtools_shared is a package you publish to pub, so I expect some of the changes to it to be reverted, but waiting for the review first.

[polina-c]

Looks good. Some questions:

1. Can you please test that Memory > Chart is still functioning?
2. What is our process around removing unused code? Is there any continuous automation that kicks it off?

[polina-c]

@elliette @kenzieschmoll @CoderDake

Can you also review?

[incendial]

What is our process around removing unused code?

You mean you want to run dcm unused code checks on bots?

[incendial]

Here is the video of working app:

Screen.Recording.2023-07-23.at.21.49.17.mov

Looks like everything works except for one overflow error (but I'm not sure whether it existed before):

Screen.Recording.2023-07-23.at.21.50.13.mov

[polina-c]

LGTM assuming others LGTM their parts

[CoderDake]

I have some concerns about devtools_shared and some other small comments

[CoderDake]

it might be better to add this to toString

[incendial]

Sure, do you have any preference regarding the format (ex. '$file:$line:$column' or '$file\n$line:$column')?

[kenzieschmoll]

'$file:$line:$column' SGTM

[incendial]

What bothers me is that file is not passed anywhere and is not used in bool operator ==(other) { and int get hashCode =>. On top of that any toString call will start with null:.

Are you sure this field should be restored?

[kenzieschmoll]

@elliette does this seem like a bug that we are not using the file parameter anywhere? I suspect this would cause weird equality issues where two SourcePosition objects appear identical because they have the same line & column & tokenPos but they are actually from two different files.

[elliette]

I don't think this is a bug. It looks like file parameter was added in PR #3448 to combine two definitions of SourcePosition in two different files (debugger_model and diagnostics_node), the second of which had file as a parameter but never used it.

SourcePosition is usually used in conjunction with some other class that contains file information (for example ScriptRef in StackFrameAndSourcePosition).

[kenzieschmoll]

Okay thanks for the input - @incendial why don't we just remove this parameter completely then as you originally did.

[incendial]

I didn't revert this change, so it's removed

[kenzieschmoll]

please remove all changes to packages/devtools_shared. The devtools_shared package is published and is used by the devtools server, which is part of the Dart SDK.

[Hixie]

@incendial Thanks for your contribution! Is this still on your radar?

[incendial]

@Hixie hi, thanks for the reminder, I hope to get back to this PR soon.

Btw, any chance we can discuss whether the Flutter team could benefit from using Dart Code Metrics?

[Hixie]

@incendial There's been some discussion of that, I forget the conclusion. I recommend bringing it up on the #hackers-framework channel of our Discord.

[kenzieschmoll]

please add a changelog entry for package:devtools_app_shared, which is also published on pub. This package is still under early development so these breaking changes are okay. Please put the changes under a new entry for version 0.0.6 and bump the package version in the pubspec.yaml to 0.0.6

[incendial]

@incendial There's been some discussion of that, I forget the conclusion. I recommend bringing it up on the #hackers-framework channel of our Discord.

@Hixie thank you, will do!

please add a changelog entry for package:devtools_app_shared, which is also published on pub.

Oh, I missed that, thanks! Will do once all other changes are approved to be merged (otherwise there might be a 0.0.6 from somebody else before that 😁).

[incendial]

@kenzieschmoll I've updated the package version to 0.0.7 and updated the changelog. Can you please take a look?
The only unresolved conversation is this one #5949 (comment). Are you sure you want this field back?

And as for failing pipelines, looks like they are failing not because of the changes introduced here. The commit before I upstreamed the main branch got all checks passed.

[kenzieschmoll]

And as for failing pipelines, looks like they are failing not because of the changes introduced here. The commit before I upstreamed the main branch got all checks passed.

If you merge master, these issues should be resolved.

[incendial]

@kenzieschmoll I think all threads are now address + the pipelines are green. Can you please take a final look? Hope we can merge this today 😁