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

Cleanup #13113

Merged
merged 21 commits into from
May 9, 2022
Merged

Cleanup #13113

merged 21 commits into from
May 9, 2022

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 7, 2022

Cleanup work:

  • Rename ErrorLogger --> DiagnosticsLogger

  • Split out part of infos.fs --> TypeHierarchy.fs

  • Split out part of SymbolHelpers.fs --> FSharpDiagnostic.fs

  • Split LowerCallsAndSeqs.fs into the three independent logical parts that it is

  • The exception type Error(...) --> SRDiagnostic. This is because this is the exception that carries diagnostics derived from SR text. Note the use of the name Error is misleading as these are also use for warnings, however it remains in the codebase as it is 1000 lines of churn. Also errorLogger remains for now, we will rename to diagsLogger in due course.

  • Rename block --> ImmutableArray. In retrospect I'm just not liking this abbreviation and it's not sufficiently helping readability. Will revisit the relevant suggestion.

@dsyme
Copy link
Contributor Author

dsyme commented May 8, 2022

@KevinRansom @vzarytovskii This is ready

@auduchinok
Copy link
Member

I think SRDiagnostic name may be hard to understand. Looking at the change after contributing to this repo for some time I don't think I know what SR stands for.

@vzarytovskii
Copy link
Member

I think SRDiagnostic name may be hard to understand. Looking at the change after contributing to this repo for some time I don't think I know what SR stands for.

If I remember correctly, it was derived from System.Resources. But I agree, it can be confusing for someone who doesn't know what's that.

@dsyme
Copy link
Contributor Author

dsyme commented May 9, 2022

I think SRDiagnostic name may be hard to understand. Looking at the change after contributing to this repo for some time I don't think I know what SR stands for.

True. Not sure what to call it otherwise. It is the primary way for diagnostics and we should really get rid of the RESX and various exception types. Maybe DiganosticWithText. The only thing I'm after here is to remove "Error" (which can be quite confusing when used as a synonym for "Diagnostic")

@dsyme dsyme changed the title [WIP] cleanup Cleanup May 9, 2022
@auduchinok
Copy link
Member

True. Not sure what to call it otherwise.

Could it be Diagnostic, GeneratedDiagnostic, or something like that?

@vzarytovskii
Copy link
Member

Maybe something like LocalizedDiagnostic? Or is it used not only with localized strings?

@dsyme
Copy link
Contributor Author

dsyme commented May 9, 2022

Maybe something like LocalizedDiagnostic? Or is it used not only with localized strings?

Hmmmm the FSStrings.resx resources are also localized I think - there is content under "src/fsharp/xlf". Historically we started localising via the FSStrings resx approach then switched

I've renamed to DiagnosticWithText for now which is pretty accurate.

@dsyme
Copy link
Contributor Author

dsyme commented May 9, 2022

Could it be Diagnostic, GeneratedDiagnostic, or something like that?

There are multiple uses of "Diagnostic" - summary below

  • The public "FSharpDiagnostic", that's ok

  • The internal "PhasedDiagnostic". This contains an exception-valued field in Diagnostic, of which one kind is this exception DiagnosticWithText. Note exceptions are generally only being used as a cheap-shot way of getting objects to store in the "Diagnostic" field of "PhasedDiagnostic", they aren't necessarily being raised. Arbitrary exceptions can however be caught and turned into PhasedDiagnostic

  • There is also a legacy "Diagnostic" used internally in CompilerDiagnostics.fs and also as part of the LegacyHostedCompilerForTesting (used only by fsharpqa suite). I've now renamed this to FormattedDiagnostic.

I think we should use DiagnosticWithText as the actual name of the exception type to make sure it's not confused with FSharpDiagnostic.

@dsyme dsyme merged commit 7f912e2 into dotnet:main May 9, 2022
vzarytovskii pushed a commit to vzarytovskii/fsharp that referenced this pull request May 10, 2022
* cleanup

* split files

* rename

* split infos.fs and SymbolHelpres.fs

* split infos.fs and SymbolHelpres.fs

* fix code formating

* rename autobox --> LowerLocalMutables

* adjust names

* block --> ImmutableArray

* format

* Error --> SRDiagnostic

* Error --> SRDiagnostic

* this -> _

* rename and cleanup

* rename Diagnostic --> FormattedDiagnostic

* format sigs

* format sigs

* fix build

* fix build
vzarytovskii added a commit to vzarytovskii/fsharp that referenced this pull request May 10, 2022
charlesroddie pushed a commit to charlesroddie/fsharp that referenced this pull request May 2, 2023
* cleanup

* split files

* rename

* split infos.fs and SymbolHelpres.fs

* split infos.fs and SymbolHelpres.fs

* fix code formating

* rename autobox --> LowerLocalMutables

* adjust names

* block --> ImmutableArray

* format

* Error --> SRDiagnostic

* Error --> SRDiagnostic

* this -> _

* rename and cleanup

* rename Diagnostic --> FormattedDiagnostic

* format sigs

* format sigs

* fix build

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

Successfully merging this pull request may close these issues.

3 participants