Skip to content

Conversation

@T-Gro
Copy link
Member

@T-Gro T-Gro commented Dec 5, 2022

When Fsharp.Core was being built with Debug. the assertion

assert(vspec.InlineInfo = inlineFlag)

at the very end of CheckExpressions was broken.
This was in main.

Reason is, the [<MethodImpl.NoInlining>] was not reflected, and only the newly introduced (F# specific) attrib_NoCompilerInliningAttribute was.

The bigger infrastructural issue is that this slipped trough in the first place.
I think we should build at least some projects also in Debug so that assertions get exercised.

@T-Gro T-Gro requested a review from a team as a code owner December 5, 2022 15:24
@T-Gro
Copy link
Member Author

T-Gro commented Dec 5, 2022

@kerams : Can you please check if this does not violate some older agreements about what [<MethodImpl.NoInlining>] or should not do?

@T-Gro
Copy link
Member Author

T-Gro commented Dec 5, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kerams
Copy link
Contributor

kerams commented Dec 5, 2022

No idea, I would have guessed MethodImplOptions.NoInlining isn't even honored, but it is https://sharplab.io/#v2:DYLgZgzgPg2gPAZQJ4QC4FMC2A6ASgVwDtUBLTdbAYQHtMAHE4dAJwRYDcSBjdCbAWXSoAFtQAmASXrAAgqlTMSAI3wYAFMjRY8RUuSq0GTVh268BQ0ZOkB5OqWqE+AOWoTCwEoS8BzAJQAfAC6ALAAUEyoAARIUWp+UQC8UQDM4eGRUQAeSTFxCQDUefFAA.

ComputeInlineFlag is also called in 2 more places if I remember right, so perhaps you might need to adjust those callsites as well, I myself don't quite follow what the issue is here.

KevinRansom
KevinRansom previously approved these changes Dec 6, 2022
abonie
abonie previously approved these changes Dec 6, 2022
vzarytovskii
vzarytovskii previously approved these changes Dec 6, 2022
@T-Gro T-Gro dismissed stale reviews from vzarytovskii, abonie, and KevinRansom via c5660e6 December 6, 2022 11:24
abonie
abonie previously approved these changes Dec 6, 2022
@T-Gro
Copy link
Member Author

T-Gro commented Dec 6, 2022

No idea, I would have guessed MethodImplOptions.NoInlining isn't even honored, but it is https://sharplab.io/#v2:DYLgZgzgPg2gPAZQJ4QC4FMC2A6ASgVwDtUBLTdbAYQHtMAHE4dAJwRYDcSBjdCbAWXSoAFtQAmASXrAAgqlTMSAI3wYAFMjRY8RUuSq0GTVh268BQ0ZOkB5OqWqE+AOWoTCwEoS8BzAJQAfAC6ALAAUEyoAARIUWp+UQC8UQDM4eGRUQAeSTFxCQDUefFAA.

ComputeInlineFlag is also called in 2 more places if I remember right, so perhaps you might need to adjust those callsites as well, I myself don't quite follow what the issue is here.

I unified all the call sites and centralized the logic, it does make sense to do it. The following happened:

[<MethodImpl(MethodImplOptions.NoInlining)>]  
let inline g() = printfn "Hey!"

In current version of the compiler, it compiles and does NOT inline - it respects the attribute and ignores the keyword.
I have code that can make it an error 3151 "This member, function or value declaration may not be declared 'inline'" ;; which does feel right. (I unified the calls about inlining and started getting new behaviour as an un-intended, but not bad, result).

I feel that we should turn it into an error as well (just like the combination of inline keyword and NoCompilerInliningAttribute is), behind a new language version flag.

abonie
abonie previously approved these changes Dec 6, 2022
psfinaki
psfinaki previously approved these changes Dec 9, 2022
@T-Gro T-Gro dismissed stale reviews from psfinaki and abonie via 1aac562 December 12, 2022 11:50
@T-Gro T-Gro requested review from abonie and psfinaki December 14, 2022 12:00
@T-Gro T-Gro merged commit 812bf3d into main Dec 22, 2022
@T-Gro T-Gro deleted the feature/fix-broken-assertion-inlineflag branch December 22, 2022 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants