Skip to content

Conversation

@psfinaki
Copy link
Contributor

@psfinaki psfinaki commented Sep 5, 2023

This:

  • removes all FixIndexerAccess code fix because I don't think it's of any use anymore
  • streamlines and speeds up actual code fix creation
  • extracts some repetitive code

@psfinaki psfinaki requested a review from a team as a code owner September 5, 2023 20:00
@psfinaki
Copy link
Contributor Author

psfinaki commented Sep 5, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

If I read things correctly, I do not really see the need to remove code which supports an error which is still being produced by the compiler codebase.
At least these two snippets in existing main are able to produce it:

                               if g.langVersion.IsExplicitlySpecifiedAs50OrBefore() then
                                    error (NotAFunctionButIndexer(denv, overallTy.Commit, vName, mExpr, mArg, false))
                                let old = not (g.langVersion.SupportsFeature LanguageFeature.IndexerNotationWithoutDot)
                                error (NotAFunctionButIndexer(denv, overallTy.Commit, vName, mExpr, mArg, old))

Either not remove at all, or remove from everywhere properly (which would mean intentionally dropping support for something specified as a LanguageFeature).

@psfinaki
Copy link
Contributor Author

psfinaki commented Sep 6, 2023

@T-Gro Yeah I was trying to understand this but failed to. Do you know which code would generate this error and how is it supposed to change after applying the code fix (given that it works)?

I mean, we don't have any tests for the diag in question and the commit introducing the code fix was pushed long ago and somehow without a PR so no example there either.

AFAIU the whole thing, even if everything somehow works, this would basically produce some code like x.[0] which we don't want to, because, well, we have the new indexer syntax (and a code fix for it). If so, this would be more confusing than helpful.

WDYT?

@T-Gro
Copy link
Member

T-Gro commented Sep 12, 2023

AFAIU the whole thing, even if everything somehow works, this would basically produce some code like x.[0] which we don't want to, because, well, we have the new indexer syntax (and a code fix for it). If so, this would be more confusing than helpful.

WDYT?

Context is everything - you have to look at it with lenses of a project being on an older F# version, for whatever reason. The compiler does support staying on a lower version, and the code in the compiler which does produce this error still exists.
The diagnostics is an compiler error, and the code fix would change it into code that actually compiles and works for that particular language version.

The codefix was already written, I don't think we should remove it just for the sake of changing something.

(I am not saying we should start writing codefixes for older F# versions. But if one was already written and we still support that older version officially, I don't see added value in removing it)

@psfinaki
Copy link
Contributor Author

In principle I don't mind keeping it - and even putting it on the wheels of the code fix framework I am developing here.

I just want to see if it works even (chances are it doesn't). But I don't understand (I tried!) what it should fix and how. If you come up with an example, I don't mind keeping it / fixing it. But if we cannot come up with a usecase, it doesn't make sense to me to keep it.

@KevinRansom maybe rings a bell for you?

@T-Gro
Copy link
Member

T-Gro commented Sep 12, 2023

In principle I don't mind keeping it - and even putting it on the wheels of the code fix framework I am developing here.

I just want to see if it works even (chances are it doesn't). But I don't understand (I tried!) what it should fix and how. If you come up with an example, I don't mind keeping it / fixing it. But if we cannot come up with a usecase, it doesn't make sense to me to keep it.

@KevinRansom maybe rings a bell for you?

Having a project with F# version pre-dating the dot-less indexer, and then doing something similar to

let x = [||1;2;3|]
let first = x[2]

Does not trigger it?

@psfinaki
Copy link
Contributor Author

@T-Gro hah right, it does get triggered (and works) on F#5, I was trying things out with F#6 only. 🤔

Agree then, shouldn't be a big deal to do things properly here, hang on.

@psfinaki psfinaki marked this pull request as draft September 13, 2023 07:43
@psfinaki psfinaki changed the title Small cleanup in code fixes Small cleanup in code fixes + tests for a legacy code fix Sep 13, 2023
@psfinaki psfinaki marked this pull request as ready for review September 13, 2023 20:13
@psfinaki psfinaki requested a review from T-Gro September 13, 2023 20:13
@psfinaki
Copy link
Contributor Author

Okay so that's done, @T-Gro thanks for being not as careless as I was here :D

@psfinaki psfinaki enabled auto-merge (squash) September 13, 2023 21:46
@psfinaki psfinaki merged commit e31702c into dotnet:main Sep 18, 2023
@psfinaki psfinaki deleted the codefixes-33 branch February 10, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants