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

Support CallerArgumentExpression (without #line) #17519

Open
wants to merge 74 commits into
base: main
Choose a base branch
from

Conversation

ijklam
Copy link
Contributor

@ijklam ijklam commented Aug 11, 2024

Description

Implements fsharp/fslang-suggestions#966

RFC fsharp/fslang-design#798

图片

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated:

Copy link
Contributor

github-actions bot commented Aug 11, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.300.md
LanguageFeatures.fsi docs/release-notes/.Language/preview.md

@ijklam
Copy link
Contributor Author

ijklam commented Aug 11, 2024

Here I encounter a problem about #line. For example, the code is saved in D:\Program.fs:

#1 "C:\\Program.fs"
System.ArgumentNullException.ThrowIfNullOrWhiteSpace("   ")  // will failed to build

// And more complicated case, repeat the file name and line number
#1 "C:\\Program.fs"
System.ArgumentNullException.ThrowIfNullOrWhiteSpace("   ")  // will failed to build

So here I want to get the original range instead of a range modified by #line . Is there anybody know how to do it?

| _ -> String.Empty
}

let getCodeText (m: range) =
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it would be possible to use ISourceText.GetSubTextString instead of all of this. I think the ISourceText for a given file will usually already be cached when this functionality is needed.

I think in theory the source text is available on cenv.tcSink.CurrentSink.Value.CurrentSourceText.Value, but maybe there's a better way to get it, or a better way to bring it in scope for this change.

Copy link
Contributor Author

@ijklam ijklam Aug 25, 2024

Choose a reason for hiding this comment

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

图片

It seems that this cannot work under dotnet fsi and under dotnet build. It is always None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would make sense to pass it in, then, as is done when checking format strings:

type FormatStringCheckContext =
{ SourceText: ISourceText
LineStartPositions: int[] }

let makeFmts (context: FormatStringCheckContext) (fragRanges: range list) (fmt: string) =
// Splits the string on interpolation holes based on fragment ranges.
// Returns a list of tuples in the form of: offset * fragment as a string * original range of the fragment
// where "offset" is the offset between beginning of the original range and where the string content begins
let numFrags = fragRanges.Length
let sourceText = context.SourceText

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would make sense to pass it in, then, as is done when checking format strings:

I guess maybe that's the same source text as in the sink... But still, maybe there's somewhere else we could put it.

@ijklam ijklam marked this pull request as ready for review March 25, 2025 11:26
@ijklam ijklam changed the title Support CallerArgumentExpression Support CallerArgumentExpression (without #line) Mar 25, 2025
@ijklam
Copy link
Contributor Author

ijklam commented Mar 25, 2025

This is ready for both the fsc and fsi, but may not work properly with the #line directive.

The #line directive processing needs LineMaps of #18049 to be available.

@T-Gro
Copy link
Member

T-Gro commented Mar 25, 2025

I propose to merge this in as is with the #line limitation, afterall it is a construct for code generators.

This PR does come under a LanguageFeature flag and we could offer the feature for real world testing under the preview version.

@@ -671,6 +671,9 @@ let main1
// Build the initial type checking environment
ReportTime tcConfig "Typecheck"

// Read the source file content for the `CallerArgumentExpression` feature
readAndStoreFileContents tcConfig sourceFiles
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a "ReportTime" section here so that we can see impact on timing in selected compilations?

Copy link
Member

Choose a reason for hiding this comment

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

Especially something bigger, like when the compiler compiles itself (this will need the --times flag to report the timings)

let readAndStoreFileContents (tcConfig: TcConfig) (sourceFiles: #seq<string>) =
for fileName in sourceFiles do
if FSharpImplFileSuffixes |> List.exists (FileSystemUtils.checkSuffix fileName) then
try
Copy link
Member

Choose a reason for hiding this comment

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

What about FileContent storing not allocated strings, but rather lazy objects?
Assumption is that majority of content will never be needed,

Copy link
Contributor

Choose a reason for hiding this comment

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

I still wonder there is any way we could pass in an existing ISourceText somehow, as suggested in #17519 (comment). But maybe it's just too hard to thread it through to where it needs to be.


/// Get the substring of the given range.
/// This can retain the line seperators in the source string.
let substring (input: string) (range: range) =
Copy link
Member

Choose a reason for hiding this comment

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

Considering the main usage of fileContentDict is to identify expressions based on a range:

  1. I would for sure make it lazy to prevent allocating massive strings which no one will read
  2. Wouldn't the algorithm be a lot simpler if that lazy storage worked with an array of lines instead of a single string? (writing that now, I do realize this would drop the ability to keep original line endings, wouldn't it?)

@@ -581,3 +581,83 @@ module Range =
| None -> mkRange file (mkPos 1 0) (mkPos 1 80)
with _ ->
mkRange file (mkPos 1 0) (mkPos 1 80)

module internal FileContent =
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel uneasy with this module.

  • It adds a global resource for a rather local feature.
  • It creates another dependency on the file system.
  • It re-reads sources from the file system that have been read before.

Getting the source text out of the checking environment as proposed by @brianrourkeboll would be much cleaner.
A (probably too ambitious) alternative would be to make this a central place for source access, to give ISourceText the role that it was meant to have and to remove file system access elsewhere.

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

Successfully merging this pull request may close these issues.

5 participants