Auto merge of rust-lang#98160 - nnethercote:mv-finish-out-of-Encoder, r=bjorn3

Move `finish` out of the `Encoder` trait.

This simplifies things, but requires making `CacheEncoder` non-generic.

(This was previously merged as commit 4 in rust-lang#94732 and then was reverted
in rust-lang#97905 because it caused a perf regression.)

r? `@ghost`

Author: bors

compiler/rustc_codegen_ssa/src/lib.rs

@@ -211,7 +211,7 @@ impl CodegenResults {
         encoder.emit_raw_bytes(&RLINK_VERSION.to_be_bytes());
         encoder.emit_str(RUSTC_VERSION.unwrap());
         Encodable::encode(codegen_results, &mut encoder);
-        encoder.finish().unwrap()
+        encoder.finish()
     }
 
     pub fn deserialize_rlink(data: Vec<u8>) -> Result<Self, String> {

compiler/rustc_incremental/src/persist/save.rs

@@ -3,7 +3,7 @@ use rustc_data_structures::sync::join;
 use rustc_middle::dep_graph::{DepGraph, SerializedDepGraph, WorkProduct, WorkProductId};
 use rustc_middle::ty::TyCtxt;
 use rustc_serialize::opaque::{FileEncodeResult, FileEncoder};
-use rustc_serialize::{Encodable as RustcEncodable, Encoder};
+use rustc_serialize::Encodable as RustcEncodable;
 use rustc_session::Session;
 use std::fs;
 

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -94,9 +94,6 @@ macro_rules! encoder_methods {
 }
 
 impl<'a, 'tcx> Encoder for EncodeContext<'a, 'tcx> {
-    type Ok = <MemEncoder as Encoder>::Ok;
-    type Err = <MemEncoder as Encoder>::Err;
-
     encoder_methods! {
         emit_usize(usize);
         emit_u128(u128);
@@ -119,10 +116,6 @@ impl<'a, 'tcx> Encoder for EncodeContext<'a, 'tcx> {
         emit_str(&str);
         emit_raw_bytes(&[u8]);
     }
-
-    fn finish(self) -> Result<Self::Ok, Self::Err> {
-        self.opaque.finish()
-    }
 }
 
 impl<'a, 'tcx, T> Encodable<EncodeContext<'a, 'tcx>> for LazyValue<T> {
@@ -2216,7 +2209,7 @@ fn encode_metadata_impl(tcx: TyCtxt<'_>) -> EncodedMetadata {
     // culminating in the `CrateRoot` which points to all of it.
     let root = ecx.encode_crate_root();
 
-    let mut result = ecx.opaque.finish().unwrap();
+    let mut result = ecx.opaque.finish();
 
     // Encode the root position.
     let header = METADATA_HEADER.len();

compiler/rustc_query_impl/src/on_disk_cache.rs

@@ -25,6 +25,7 @@ use rustc_span::hygiene::{
 use rustc_span::source_map::{SourceMap, StableSourceFileId};
 use rustc_span::CachingSourceMapView;
 use rustc_span::{BytePos, ExpnData, ExpnHash, Pos, SourceFile, Span};
+use std::io;
 use std::mem;
 
 const TAG_FILE_FOOTER: u128 = 0xC0FFEE_C0FFEE_C0FFEE_C0FFEE_C0FFEE;
@@ -807,21 +808,10 @@ impl_ref_decoder! {<'tcx>
 
 //- ENCODING -------------------------------------------------------------------
 
-pub trait OpaqueEncoder: Encoder {
-    fn position(&self) -> usize;
-}
-
-impl OpaqueEncoder for FileEncoder {
-    #[inline]
-    fn position(&self) -> usize {
-        FileEncoder::position(self)
-    }
-}
-
 /// An encoder that can write to the incremental compilation cache.
-pub struct CacheEncoder<'a, 'tcx, E: OpaqueEncoder> {
+pub struct CacheEncoder<'a, 'tcx> {
     tcx: TyCtxt<'tcx>,
-    encoder: E,
+    encoder: FileEncoder,
     type_shorthands: FxHashMap<Ty<'tcx>, usize>,
     predicate_shorthands: FxHashMap<ty::PredicateKind<'tcx>, usize>,
     interpret_allocs: FxIndexSet<interpret::AllocId>,
@@ -830,10 +820,7 @@ pub struct CacheEncoder<'a, 'tcx, E: OpaqueEncoder> {
     hygiene_context: &'a HygieneEncodeContext,
 }
 
-impl<'a, 'tcx, E> CacheEncoder<'a, 'tcx, E>
-where
-    E: OpaqueEncoder,
-{
+impl<'a, 'tcx> CacheEncoder<'a, 'tcx> {
     fn source_file_index(&mut self, source_file: Lrc<SourceFile>) -> SourceFileIndex {
         self.file_to_file_index[&(&*source_file as *const SourceFile)]
     }
@@ -852,32 +839,27 @@ where
         let end_pos = self.position();
         ((end_pos - start_pos) as u64).encode(self);
     }
+
+    fn finish(self) -> Result<usize, io::Error> {
+        self.encoder.finish()
+    }
 }
 
-impl<'a, 'tcx, E> Encodable<CacheEncoder<'a, 'tcx, E>> for SyntaxContext
-where
-    E: OpaqueEncoder,
-{
-    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx, E>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for SyntaxContext {
+    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx>) {
         rustc_span::hygiene::raw_encode_syntax_context(*self, s.hygiene_context, s);
     }
 }
 
-impl<'a, 'tcx, E> Encodable<CacheEncoder<'a, 'tcx, E>> for ExpnId
-where
-    E: OpaqueEncoder,
-{
-    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx, E>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for ExpnId {
+    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx>) {
         s.hygiene_context.schedule_expn_data_for_encoding(*self);
         self.expn_hash().encode(s);
     }
 }
 
-impl<'a, 'tcx, E> Encodable<CacheEncoder<'a, 'tcx, E>> for Span
-where
-    E: OpaqueEncoder,
-{
-    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx, E>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for Span {
+    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx>) {
         let span_data = self.data_untracked();
         span_data.ctxt.encode(s);
         span_data.parent.encode(s);
@@ -920,10 +902,7 @@ where
     }
 }
 
-impl<'a, 'tcx, E> TyEncoder for CacheEncoder<'a, 'tcx, E>
-where
-    E: OpaqueEncoder,
-{
+impl<'a, 'tcx> TyEncoder for CacheEncoder<'a, 'tcx> {
     type I = TyCtxt<'tcx>;
     const CLEAR_CROSS_CRATE: bool = false;
 
@@ -943,29 +922,20 @@ where
     }
 }
 
-impl<'a, 'tcx, E> Encodable<CacheEncoder<'a, 'tcx, E>> for CrateNum
-where
-    E: OpaqueEncoder,
-{
-    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx, E>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for CrateNum {
+    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx>) {
         s.tcx.stable_crate_id(*self).encode(s);
     }
 }
 
-impl<'a, 'tcx, E> Encodable<CacheEncoder<'a, 'tcx, E>> for DefId
-where
-    E: OpaqueEncoder,
-{
-    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx, E>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for DefId {
+    fn encode(&self, s: &mut CacheEncoder<'a, 'tcx>) {
         s.tcx.def_path_hash(*self).encode(s);
     }
 }
 
-impl<'a, 'tcx, E> Encodable<CacheEncoder<'a, 'tcx, E>> for DefIndex
-where
-    E: OpaqueEncoder,
-{
-    fn encode(&self, _: &mut CacheEncoder<'a, 'tcx, E>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for DefIndex {
+    fn encode(&self, _: &mut CacheEncoder<'a, 'tcx>) {
         bug!("encoding `DefIndex` without context");
     }
 }
@@ -979,13 +949,7 @@ macro_rules! encoder_methods {
     }
 }
 
-impl<'a, 'tcx, E> Encoder for CacheEncoder<'a, 'tcx, E>
-where
-    E: OpaqueEncoder,
-{
-    type Ok = E::Ok;
-    type Err = E::Err;
-
+impl<'a, 'tcx> Encoder for CacheEncoder<'a, 'tcx> {
     encoder_methods! {
         emit_usize(usize);
         emit_u128(u128);
@@ -1008,30 +972,26 @@ where
         emit_str(&str);
         emit_raw_bytes(&[u8]);
     }
-
-    fn finish(self) -> Result<E::Ok, E::Err> {
-        self.encoder.finish()
-    }
 }
 
 // This ensures that the `Encodable<opaque::FileEncoder>::encode` specialization for byte slices
 // is used when a `CacheEncoder` having an `opaque::FileEncoder` is passed to `Encodable::encode`.
 // Unfortunately, we have to manually opt into specializations this way, given how `CacheEncoder`
 // and the encoding traits currently work.
-impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx, FileEncoder>> for [u8] {
-    fn encode(&self, e: &mut CacheEncoder<'a, 'tcx, FileEncoder>) {
+impl<'a, 'tcx> Encodable<CacheEncoder<'a, 'tcx>> for [u8] {
+    fn encode(&self, e: &mut CacheEncoder<'a, 'tcx>) {
         self.encode(&mut e.encoder);
     }
 }
 
 pub fn encode_query_results<'a, 'tcx, CTX, Q>(
     tcx: CTX,
-    encoder: &mut CacheEncoder<'a, 'tcx, FileEncoder>,
+    encoder: &mut CacheEncoder<'a, 'tcx>,
     query_result_index: &mut EncodedDepNodeIndex,
 ) where
     CTX: QueryContext + 'tcx,
     Q: super::QueryDescription<CTX>,
-    Q::Value: Encodable<CacheEncoder<'a, 'tcx, FileEncoder>>,
+    Q::Value: Encodable<CacheEncoder<'a, 'tcx>>,
 {
     let _timer = tcx
         .dep_context()

compiler/rustc_query_impl/src/plumbing.rs

@@ -12,7 +12,6 @@ use rustc_query_system::query::{QueryContext, QueryJobId, QueryMap, QuerySideEff
 use rustc_data_structures::sync::Lock;
 use rustc_data_structures::thin_vec::ThinVec;
 use rustc_errors::{Diagnostic, Handler};
-use rustc_serialize::opaque;
 
 use std::any::Any;
 use std::num::NonZeroU64;
@@ -140,7 +139,7 @@ impl<'tcx> QueryCtxt<'tcx> {
 
     pub(super) fn encode_query_results(
         self,
-        encoder: &mut on_disk_cache::CacheEncoder<'_, 'tcx, opaque::FileEncoder>,
+        encoder: &mut on_disk_cache::CacheEncoder<'_, 'tcx>,
         query_result_index: &mut on_disk_cache::EncodedDepNodeIndex,
     ) {
         macro_rules! encode_queries {

compiler/rustc_query_system/src/dep_graph/serialized.rs

@@ -20,7 +20,7 @@ use rustc_data_structures::profiling::SelfProfilerRef;
 use rustc_data_structures::sync::Lock;
 use rustc_index::vec::{Idx, IndexVec};
 use rustc_serialize::opaque::{FileEncodeResult, FileEncoder, IntEncodedWithFixedSize, MemDecoder};
-use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
+use rustc_serialize::{Decodable, Decoder, Encodable};
 use smallvec::SmallVec;
 use std::convert::TryInto;
 

compiler/rustc_serialize/src/opaque.rs

@@ -24,6 +24,10 @@ impl MemEncoder {
     pub fn position(&self) -> usize {
         self.data.len()
     }
+
+    pub fn finish(self) -> Vec<u8> {
+        self.data
+    }
 }
 
 macro_rules! write_leb128 {
@@ -54,9 +58,6 @@ macro_rules! write_leb128 {
 const STR_SENTINEL: u8 = 0xC1;
 
 impl Encoder for MemEncoder {
-    type Ok = Vec<u8>;
-    type Err = !;
-
     #[inline]
     fn emit_usize(&mut self, v: usize) {
         write_leb128!(self, v, usize, write_usize_leb128)
@@ -150,10 +151,6 @@ impl Encoder for MemEncoder {
     fn emit_raw_bytes(&mut self, s: &[u8]) {
         self.data.extend_from_slice(s);
     }
-
-    fn finish(self) -> Result<Self::Ok, Self::Err> {
-        Ok(self.data)
-    }
 }
 
 pub type FileEncodeResult = Result<usize, io::Error>;
@@ -389,6 +386,13 @@ impl FileEncoder {
             }
         }
     }
+
+    pub fn finish(mut self) -> Result<usize, io::Error> {
+        self.flush();
+
+        let res = std::mem::replace(&mut self.res, Ok(()));
+        res.map(|()| self.position())
+    }
 }
 
 impl Drop for FileEncoder {
@@ -426,9 +430,6 @@ macro_rules! file_encoder_write_leb128 {
 }
 
 impl Encoder for FileEncoder {
-    type Ok = usize;
-    type Err = io::Error;
-
     #[inline]
     fn emit_usize(&mut self, v: usize) {
         file_encoder_write_leb128!(self, v, usize, write_usize_leb128)
@@ -522,13 +523,6 @@ impl Encoder for FileEncoder {
     fn emit_raw_bytes(&mut self, s: &[u8]) {
         self.write_all(s);
     }
-
-    fn finish(mut self) -> Result<usize, io::Error> {
-        self.flush();
-
-        let res = std::mem::replace(&mut self.res, Ok(()));
-        res.map(|()| self.position())
-    }
 }
 
 // -----------------------------------------------------------------------------

compiler/rustc_serialize/src/serialize.rs

@@ -18,13 +18,10 @@ use std::sync::Arc;
 /// is pervasive and has non-trivial cost. Instead, impls of this trait must
 /// implement a delayed error handling strategy. If a failure occurs, they
 /// should record this internally, and all subsequent encoding operations can
-/// be processed or ignored, whichever is appropriate. Then when `finish()` is
-/// called, an error result should be returned to indicate the failure. If no
-/// failures occurred, then `finish()` should return a success result.
+/// be processed or ignored, whichever is appropriate. Then they should provide
+/// a `finish` method that finishes up encoding. If the encoder is fallible,
+/// `finish` should return a `Result` that indicates success or failure.
 pub trait Encoder {
-    type Ok;
-    type Err;
-
     // Primitive types:
     fn emit_usize(&mut self, v: usize);
     fn emit_u128(&mut self, v: u128);
@@ -64,9 +61,6 @@ pub trait Encoder {
     fn emit_fieldless_enum_variant<const ID: usize>(&mut self) {
         self.emit_usize(ID)
     }
-
-    // Consume the encoder, getting the result.
-    fn finish(self) -> Result<Self::Ok, Self::Err>;
 }
 
 // Note: all the methods in this trait are infallible, which may be surprising.

compiler/rustc_serialize/tests/opaque.rs

@@ -2,7 +2,7 @@
 
 use rustc_macros::{Decodable, Encodable};
 use rustc_serialize::opaque::{MemDecoder, MemEncoder};
-use rustc_serialize::{Decodable, Encodable, Encoder as EncoderTrait};
+use rustc_serialize::{Decodable, Encodable};
 use std::fmt::Debug;
 
 #[derive(PartialEq, Clone, Debug, Encodable, Decodable)]
@@ -38,7 +38,7 @@ fn check_round_trip<
         Encodable::encode(value, &mut encoder);
     }
 
-    let data = encoder.finish().unwrap();
+    let data = encoder.finish();
     let mut decoder = MemDecoder::new(&data[..], 0);
 
     for value in values {

src/librustdoc/scrape_examples.rs

@@ -18,7 +18,7 @@ use rustc_middle::hir::nested_filter;
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_serialize::{
     opaque::{FileEncoder, MemDecoder},
-    Decodable, Encodable, Encoder,
+    Decodable, Encodable,
 };
 use rustc_session::getopts;
 use rustc_span::{