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

Add #[default_method_body_is_const] #86857

Merged
merged 12 commits into from
Jul 13, 2021
Merged

Conversation

fee1-dead
Copy link
Member

@rustbot label F-const_trait_impl

@rustbot rustbot added the F-const_trait_impl `#![feature(const_trait_impl)]` label Jul 4, 2021
@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2021
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

I'm not familiar with const fns, but I'd expect that functions marked #[default_method_body_is_const] must abide by the same restrictions as const fns (you can't call non-const functions, etc). Apparently const checking occurs in rustc_mir::transform::check_consts, you might want to take a look there.

Also, IIUC a function with #[default_method_body_is_const] is considered a const fn only in a const impl. I know nothing about how the compiler handles calls to const fns, but I'd expect that someone needs to point out that, hey, that function is const here. Can't help much with that though, so I'll pass the PR to someone that actually understands the mechanisms at play 😅

Left a few nits as well.

compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/check_const.rs Outdated Show resolved Hide resolved
@LeSeulArtichaut
Copy link
Contributor

r? @RalfJung do you want to take this?

@fee1-dead
Copy link
Member Author

fee1-dead commented Jul 5, 2021

functions marked #[default_method_body_is_const] must abide by the same restrictions as const fns

I agree, the problem is with the current changes a definition below would not error:

trait DefaultNonConstIsConst {
    #[default_method_body_is_const]
    fn i_lied() {
        println!("WARNING: NOT CONST")
    }
}

I will look into this, but I would also appreciate it if someone can point me in the right direction.

EDIT: I don't know where I can make it check a default function of trait, if marked by the attribute...

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2021

I'm afraid this week I am rather swamped, and this is not code I have ever touched. (I didn't even know there is const checking going on in "rustc passes".)
Cc @rust-lang/wg-const-eval

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2021

Is there somewhere a description of the point of this feature and how it fits in the larger story of const fn in traits?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 5, 2021

EDIT: I don't know where I can make it check a default function of trait, if marked by the attribute...

I'm not sure if this does the trick or whether default bodies have other gotchas, but it should suffice to check in is_const_fn_raw
whether the function has your new attribute and treat it as const then

@fee1-dead
Copy link
Member Author

fee1-dead commented Jul 6, 2021

That does not seem to work. Does const checking skip functions of traits?

EDIT: it worked, I put the "lying" trait into another file instead of the one containing errors.

@fee1-dead
Copy link
Member Author

It does the const check, but it has some issues, as shown in the code below:

#![feature(const_trait_impl)]
#![allow(incomplete_features)]

#![feature(const_fn_trait_bound)] // <- why is this needed? 

trait ConstDefaultFn: Sized {
    fn b(self);

    #[default_method_body_is_const]
    fn a(self) {
        self.b(); // Should not error here as this is only in `impl const ConstDefaultFn for Whatever`
    }
}

struct NonConstImpl;
struct ConstImpl;

impl ConstDefaultFn for NonConstImpl {
    fn b(self) {}
}
impl const ConstDefaultFn for ConstImpl {
    fn b(self) {}
}

const fn test() {
    NonConstImpl.a(); // Should error: `fn a(self) of NonConstImpl is not const`
    ConstImpl.a();
}

fn main() {}
error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
 --> src/test/ui/rfc-2632-const-trait-impl/const-default-method-bodies.rs:9:9
  |
9 |         self.b();
  |         ^^^^^^^^

error: aborting due to previous error

This is beyond my knowledge of the const-checking system, and I am not sure what to do next. I will push a commit with the (failed) test and mark this as a draft.

@fee1-dead fee1-dead marked this pull request as draft July 6, 2021 05:06
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2021

#![feature(const_fn_trait_bound)] // <- why is this needed?

maybe due to the implicit Self: ConstDefaultFn bound? unsure, leave a fixme on the test

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2021

self.b(); // Should not error here as this is only in impl const ConstDefaultFn for Whatever

this is tricky. we need to treat the Self type inside default_method_body_is_const methods as const ConstDefaultFn. I'm not sure what the best way for doing this is, checking now...

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2021

So...

self.check_op(ops::FnCallNonConst);
is where things abort in this test. What we could do is skip that test when caller has the attribute and callee is a method on the same trait as caller. I'm not sure how super traits work with impl const Trait right now, but once that works, we can include methods from super traits, too.

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member Author

fee1-dead commented Jul 7, 2021

Done, I don't think making the trait function const in is_const_fn_raw would be desirable, though. It can be unsound because it would only be const when used on impl const. The following test case:

NonConstImpl.a();

currently does not error. If it is special-cased in rustc_mir::transform::check_consts (to fix the test), then I speculate that cross-crate usages would fail. So it must not be treated as const in is_const_fn_raw, and should be only special-cased in the module that selects the functions to be const-checked.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 9, 2021

I don't think making the trait function const in is_const_fn_raw would be desirable, though

aw. why is this happening. Probably because of the way default method bodies are desugared. They are in fact using something like specialization in the background, so we get something like

trait ConstDefaultFn: Sized {
    fn b(self);
    fn a(self);
}

default impl<T> const ConstDefaultFn for T {
    fn a(self) {
        self.b();
    }
}

struct NonConstImpl;
struct ConstImpl;

impl ConstDefaultFn for NonConstImpl {
    fn b(self) {}
}

impl const ConstDefaultFn for ConstImpl {
    fn b(self) {}
}

and the the last two impls never get their own DefId for their a function, but instead the definition of a has one DefId that is used in both of the impls.

So neither the attribute system we have now, nor the desugared specialization scheme will work as desired. a's body still must be treated as const by the const checker. That part is easy, just duplicate

BodyOwnerKind::Fn if self.tcx.is_const_fn_raw(did.to_def_id()) => ConstContext::ConstFn,
and replace the guard with the check you have in is_const_fn right now. This treats the body as const fn, which is exactly what we want. The body will now get const checked. We should probably look at all call sites of body_const_context to see if this is indeed how this function is used, and then document that it may only be used for body information, never to assume things about the signature.

Now we need to figure out a way to be able to call functions that have the attribute.

It is not sufficient to, just before your newly added same-trait check, check that the callee has the attribute and return true. This would effectively make the attribute equivalent to having const in the fn signature, but "only" from the const checker's perspective. That is exactly the problem we have right now.

So... my next idea was to check if the impl of the trait for the Self type is const. Not sure if that is problematic, I don't believe it can cause problems even if the Self is still generic, as in that case we assume an impl const. For this, we can't just get the impl block in which the function is defined (via https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.impl_of_method), as it is defined in the trait and not the impl const Trait.

So... I guess we can use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.for_each_relevant_impl to iterate over all impls of the trait for the type (remember, with specialization there can be multiple) and check if they are all impl const.

Finally, the above changes have sufficiently changed things up for CTFE's dynamic checks to bail out when trying to eval such functions (as they use is_const_fn_raw), so we need to tell CTFE that "yes you may call non-const fn, if they have the attribute". I think doing something at

if !ecx.tcx.is_const_fn_raw(def.did) {
should be sufficient, but you may run into more failures.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member Author

fee1-dead commented Jul 10, 2021

Thank you for the detailed mentor instructions. I have applied all of them and will mark this as ready for review now.

@fee1-dead fee1-dead marked this pull request as ready for review July 10, 2021 12:21
@RalfJung
Copy link
Member

r? @oli-obk
I am lost here.^^

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 11, 2021
@fee1-dead
Copy link
Member Author

Other tests suggest that non-x86 arches have the correct size. This error is spurious, my code has not changed dylib stuff...

@bors retry rollup=maybe

@bors
Copy link
Contributor

bors commented Jul 11, 2021

@fee1-dead: 🔑 Insufficient privileges: not in try users

@oli-obk
Copy link
Contributor

oli-obk commented Jul 11, 2021

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2021
@bors
Copy link
Contributor

bors commented Jul 11, 2021

⌛ Testing commit 7c9e214 with merge 622ec7f53c6edc9f546196a035c2afb023674b70...

@rust-log-analyzer
Copy link
Collaborator

The job i686-msvc-2 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
diff of stderr:

8    = note: see issue #58713 <https://github.com/rust-lang/rust/issues/58713> for more information
9 
10 error: multiple definitions of external function `f` from library `foo.dll` have different calling conventions
-   --> $DIR/multiple-definitions.rs:8:5
12    |
12    |
- LL |     fn f(x: i32);
-    |     ^^^^^^^^^^^^^
+ LL |         fn f(x: i32);
15 
16 error: aborting due to previous error; 1 warning emitted
17 

---
To only update this specific test, also pass `--test-args rfc-2627-raw-dylib\multiple-definitions.rs`

error: 1 errors occurred comparing output.
status: exit code: 1
command: PATH="D:\a\rust\rust\build\i686-pc-windows-msvc\stage2\bin;C:\Program Files (x86)\Windows Kits\10\bin\x64;C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64;C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30037\bin\HostX64\x64;C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30037\bin\HostX64\x86;D:\a\rust\rust\build\i686-pc-windows-msvc\stage0-bootstrap-tools\i686-pc-windows-msvc\release\deps;D:\a\rust\rust\build\i686-pc-windows-msvc\stage0\bin;D:\a\rust\rust\ninja;D:\a\rust\rust\msys2\mingw32\bin;C:\hostedtoolcache\windows\Python\3.9.5\x64\Scripts;C:\hostedtoolcache\windows\Python\3.9.5\x64;C:\msys64\usr\bin;D:\a\rust\rust\sccache;C:\Users\runneradmin\.dotnet\tools;C:\Program Files\MongoDB\Server\4.4\bin;C:\aliyun-cli;C:\vcpkg;C:\cf-cli;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.7.1\x64;C:\tools\ghc-9.0.1\bin;C:\Program Files\dotnet;C:\mysql-5.7.21-winx64\bin;C:\Program Files\R\R-4.1.0\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;C:\Program Files (x86)\sbt\bin;C:\Rust\.cargo\bin;C:\Program Files (x86)\GitHub CLI;C:\Program Files\Git\bin;C:\Program Files (x86)\pipx_bin;C:\hostedtoolcache\windows\go\1.15.13\x64\bin;C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts;C:\hostedtoolcache\windows\Python\3.7.9\x64;C:\hostedtoolcache\windows\Ruby\2.5.9\x64\bin;C:\hostedtoolcache\windows\Java_Adopt_jdk\8.0.292-10\x64\bin;C:\npm\prefix;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\ProgramData\Chocolatey\bin;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\Docker;C:\Program Files\PowerShell\7;C:\Program Files\dotnet;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\150\DTS\Binn;C:\Program Files\nodejs;C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin;C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.1\bin;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files\OpenSSL\bin;C:\Strawberry\c\bin;C:\Strawberry\perl\site\bin;C:\Strawberry\perl\bin;C:\Program Files\Git\cmd;C:\Program Files\Git\mingw64\bin;C:\Program Files\Git\usr\bin;C:\tools\php;C:\Program Files (x86)\sbt\bin;C:\Program Files\TortoiseSVN\bin;C:\SeleniumWebDrivers\ChromeDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\Program Files\CMake\bin;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files (x86)\Microsoft BizTalk Server;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustc.exe" "D:\\a\\rust\\rust\\src/test\\ui\\rfc-2627-raw-dylib\\multiple-definitions.rs" "-Zthreads=1" "--target=i686-pc-windows-msvc" "--error-format" "json" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\ui\\rfc-2627-raw-dylib\\multiple-definitions" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--crate-type" "lib" "--emit" "link" "-L" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\ui\\rfc-2627-raw-dylib\\multiple-definitions\\auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
warning: the feature `raw_dylib` is incomplete and may not be safe to use and/or cause compiler crashes
  --> D:\a\rust\rust\src/test\ui\rfc-2627-raw-dylib\multiple-definitions.rs:4:12
   |
LL | #![feature(raw_dylib)]
   |
   = note: `#[warn(incomplete_features)]` on by default
   = note: see issue #58713 <https://github.com/rust-lang/rust/issues/58713> for more information


error: multiple definitions of external function `f` from library `foo.dll` have different calling conventions
  --> D:\a\rust\rust\src/test\ui\rfc-2627-raw-dylib\multiple-definitions.rs:15:9
   |
LL |         fn f(x: i32);

error: aborting due to previous error; 1 warning emitted


---
    [ui] ui\rfc-2627-raw-dylib\multiple-definitions.rs

test result: FAILED. 11937 passed; 1 failed; 183 ignored; 0 measured; 0 filtered out; finished in 403.35s

Some tests failed in compiletest suite=ui mode=ui host=i686-pc-windows-msvc target=i686-pc-windows-msvc


command did not execute successfully: "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage0-tools-bin\\compiletest.exe" "--compile-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin" "--run-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib" "--rustc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustc.exe" "--src-base" "D:\\a\\rust\\rust\\src/test\\ui" "--build-base" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\ui" "--stage-id" "stage2-i686-pc-windows-msvc" "--suite" "ui" "--mode" "ui" "--target" "i686-pc-windows-msvc" "--host" "i686-pc-windows-msvc" "--llvm-filecheck" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\llvm\\build\\bin\\FileCheck.exe" "--nodejs" "C:\\Program Files\\nodejs\\node" "--npm" "C:\\Program Files\\nodejs\\npm" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--docck-python" "C:\\hostedtoolcache\\windows\\Python\\3.9.5\\x64\\python3.exe" "--lldb-python" "C:\\hostedtoolcache\\windows\\Python\\3.9.5\\x64\\python3.exe" "--gdb" "C:\\msys64\\usr\\bin\\gdb" "--llvm-version" "12.0.1-rust-1.55.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:38:38
Build completed unsuccessfully in 0:38:38
make: *** [Makefile:74: ci-subset-2] Error 1

@bors
Copy link
Contributor

bors commented Jul 11, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 11, 2021
@fee1-dead

This comment has been minimized.

@fee1-dead
Copy link
Member Author

@oli-obk please tell bors to retry this, thx

@RalfJung
Copy link
Member

This is #87084, not sure if a retry will help.
@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2021
@bors
Copy link
Contributor

bors commented Jul 13, 2021

⌛ Testing commit 7c9e214 with merge 394804b...

@bors
Copy link
Contributor

bors commented Jul 13, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 394804b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2021
@bors bors merged commit 394804b into rust-lang:master Jul 13, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 13, 2021
@fee1-dead fee1-dead deleted the add-attr branch August 19, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_trait_impl `#![feature(const_trait_impl)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants