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

Always use typeEquivAux EraseMeasures (for integral range optimizations) #17048

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Apr 15, 2024

Description

Followup to #16650 and #17040.

Checklist

  • Test cases updated.
  • Release notes entry updated.

@brianrourkeboll brianrourkeboll marked this pull request as ready for review April 15, 2024 16:39
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner April 15, 2024 16:39
Copy link
Contributor

github-actions bot commented Apr 15, 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/8.0.400.md

@T-Gro T-Gro changed the title Always use typeEquivAux EraseMeasures Always use typeEquivAux EraseMeasures (for integral range optimizations) Apr 16, 2024
@psfinaki
Copy link
Member

Is it worth adding any more tests here compared to the ones in #17040?

@brianrourkeboll
Copy link
Contributor Author

@psfinaki

Is it worth adding any more tests here compared to the ones in #17040?

The tests added in #17040 actually already showed this problem — that's why the baselines needed to be updated in this PR. I just didn't notice that there were additional places affected by this in #17040 because I was focusing on (1) whether the code compiled at all and (2) that the generated IL used the new for/while-loops instead of calling the range operator.

I think that these tests (the ones added in #17040 and updated here) are enough to show that the class of bug caused by using stripMeasuresFromTy instead of typeEquivAux EraseMeasures is fixed.

If we wanted to be more thorough in general, though, we really should add duplicates of all the existing emitted IL tests for integral ranges but with units of measure — although we would then be faced with #17046 and fsharp/fslang-suggestions#535. We might even add additional tests that asserted that the emitted IL for the unit-less versions matched the emitted IL for the versions with units.

As a sanity check, we could also/alternatively add duplicates with units of measure of all the existing non-emitted-IL tests for ranges, like these, though again we'd need to work around fsharp/fslang-suggestions#535.

@psfinaki
Copy link
Member

@brianrourkeboll thanks for the detailed explanation. I think this can go as is then, the tests you suggest would be helpful but definitely a separate and potentially big effort.

Thanks!

@psfinaki psfinaki enabled auto-merge (squash) April 16, 2024 15:30
@psfinaki psfinaki merged commit ed08121 into dotnet:main Apr 16, 2024
32 checks passed
@brianrourkeboll brianrourkeboll deleted the units-of-measure-again branch April 16, 2024 15:33
psfinaki added a commit that referenced this pull request Apr 17, 2024
* Disallow calling abstract methods directly on interfaces (#17021)

* Disallow calling abstract methods directly on interfaces

* More tests

* IWSAMs are not supported by NET472

* Update src/Compiler/Checking/ConstraintSolver.fs

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* fix typos

* looking for the right check

* Add comments

* move release notes

* Add a new error number and message

* Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Improve error message

---------

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Always use `typeEquivAux EraseMeasures` (for  integral range optimizations) (#17048)

* Always use `typeEquivAux EraseMeasures`

* Update release notes

* Update baselines

---------

Co-authored-by: Petr <psfinaki@users.noreply.github.com>

---------

Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com>
Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>
Co-authored-by: Petr <psfinaki@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
psfinaki added a commit that referenced this pull request Apr 17, 2024
* Disallow calling abstract methods directly on interfaces (#17021)

* Disallow calling abstract methods directly on interfaces

* More tests

* IWSAMs are not supported by NET472

* Update src/Compiler/Checking/ConstraintSolver.fs

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* fix typos

* looking for the right check

* Add comments

* move release notes

* Add a new error number and message

* Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Improve error message

---------

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Always use `typeEquivAux EraseMeasures` (for  integral range optimizations) (#17048)

* Always use `typeEquivAux EraseMeasures`

* Update release notes

* Update baselines

---------

Co-authored-by: Petr <psfinaki@users.noreply.github.com>

---------

Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com>
Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>
Co-authored-by: Petr <psfinaki@users.noreply.github.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
psfinaki added a commit that referenced this pull request Apr 18, 2024
* Disallow calling abstract methods directly on interfaces (#17021)

* Disallow calling abstract methods directly on interfaces

* More tests

* IWSAMs are not supported by NET472

* Update src/Compiler/Checking/ConstraintSolver.fs

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* fix typos

* looking for the right check

* Add comments

* move release notes

* Add a new error number and message

* Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Improve error message

---------

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Always use `typeEquivAux EraseMeasures` (for  integral range optimizations) (#17048)

* Always use `typeEquivAux EraseMeasures`

* Update release notes

* Update baselines

---------

Co-authored-by: Petr <psfinaki@users.noreply.github.com>

* Error message that explicitly disallowed static abstract members in classes. (#17055)

* WIP

* Error message that explicitly disallowed static abstract methods in abstract classes

* release notes

* SynTypeDefnKind.Class

* Fix #16761 (#17047)

* Fix #16761

* Fully async version + ignore cancellation on external navigation

* Automated command ran: fantomas

  Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

---------

Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com>
Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>
Co-authored-by: Petr <psfinaki@users.noreply.github.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
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