Fix transitive check

Summary:
Fix transitive checking of stubs.

It is not sound to assert that a reference is valid if it has already been visited, because the whole cycle may prove to be invalid later on.

The proposed fix is to store "maybe invalid" references (those that have already been visited), and resolve them at once at the end, once information from non-recursive branches has been propagated.

Reviewed By: ilya-klyuchnikov

Differential Revision: D60903935

fbshipit-source-id: 46102b7583f1b98cdc081803cce97dcfe44e17da

crates/elp/src/resources/test/eqwalizer_tests/check/recursive_aliases.pretty

@@ -433,4 +433,22 @@ error: reference_to_invalid_type
 
 See https://fb.me/eqwalizer_errors#reference_to_invalid_type
 
-40 ERRORS
+error: reference_to_invalid_type
+    ┌─ check/src/recursive_aliases.erl:513:1
+    │  
+513 │ ╭ -type invalid_transitive() ::
+514 │ │   {a, invalid_rec()}.
+    │ ╰────────────────────^ invalid_transitive/0 references type with invalid definition: invalid_rec/0
+
+See https://fb.me/eqwalizer_errors#reference_to_invalid_type
+
+error: reference_to_invalid_type
+    ┌─ check/src/recursive_aliases.erl:516:1
+    │  
+516 │ ╭ -spec use_invalid
+517 │ │   (invalid_transitive()) -> a.
+    │ ╰─────────────────────────────^ use_invalid/1 references type with invalid definition: invalid_transitive/0
+
+See https://fb.me/eqwalizer_errors#reference_to_invalid_type
+
+42 ERRORS

crates/eqwalizer/src/ast/trans_valid.rs

@@ -59,6 +59,7 @@ pub struct TransitiveChecker<'d> {
     module: SmolStr,
     in_progress: FxHashSet<Ref>,
     invalid_refs: FxHashMap<Ref, FxHashSet<Ref>>,
+    maybe_invalid_refs: FxHashMap<Ref, FxHashSet<Ref>>,
 }
 
 impl TransitiveChecker<'_> {
@@ -73,6 +74,7 @@ impl TransitiveChecker<'_> {
             module,
             in_progress: FxHashSet::default(),
             invalid_refs: FxHashMap::default(),
+            maybe_invalid_refs: FxHashMap::default(),
         }
     }
 
@@ -167,6 +169,7 @@ impl TransitiveChecker<'_> {
             &mut invalids,
             &self.module.clone(),
             &Type::FunType(spec.ty.to_owned()),
+            None,
         )?;
         if !invalids.is_empty() {
             let references = invalids.iter().map(|rref| self.show(rref)).collect();
@@ -221,6 +224,7 @@ impl TransitiveChecker<'_> {
                 &mut invalids,
                 &self.module.clone(),
                 &Type::FunType(ty.to_owned()),
+                None,
             )?;
         }
         if !invalids.is_empty() {
@@ -253,6 +257,7 @@ impl TransitiveChecker<'_> {
                 &mut invalids,
                 &self.module.clone(),
                 &Type::FunType(ty.to_owned()),
+                None,
             )?;
             if invalids.is_empty() {
                 filtered_tys.push(ty.clone())
@@ -268,7 +273,35 @@ impl TransitiveChecker<'_> {
     }
 
     fn is_valid(&mut self, rref: &Ref) -> Result<bool, TransitiveCheckError> {
+        let maybe_valid = self.is_maybe_valid(rref, None)?;
+        let mut resolved_invalids = FxHashSet::default();
+        if let Some(maybe_invalids) = self.maybe_invalid_refs.remove(rref) {
+            for maybe_invalid in maybe_invalids.iter() {
+                if !self.is_valid(maybe_invalid)? {
+                    resolved_invalids.insert(maybe_invalid.clone());
+                }
+            }
+        }
+        let has_no_resolved_invalids = resolved_invalids.is_empty();
+        self.invalid_refs
+            .entry(rref.clone())
+            .or_default()
+            .extend(resolved_invalids);
+        Ok(maybe_valid && has_no_resolved_invalids)
+    }
+
+    fn is_maybe_valid(
+        &mut self,
+        rref: &Ref,
+        parent_ref: Option<&Ref>,
+    ) -> Result<bool, TransitiveCheckError> {
         if self.in_progress.contains(rref) {
+            if let Some(pref) = parent_ref {
+                self.maybe_invalid_refs
+                    .entry(pref.clone())
+                    .or_default()
+                    .insert(rref.clone());
+            }
             return Ok(true);
         }
         if let Some(invs) = self.invalid_refs.get(rref) {
@@ -291,12 +324,14 @@ impl TransitiveChecker<'_> {
                             &mut invalids,
                             &rid.module,
                             &tdecl.body,
+                            Some(rref),
                         )?,
                         None => match stub.private_opaques.get(&id) {
                             Some(tdecl) => self.collect_invalid_references(
                                 &mut invalids,
                                 &rid.module,
                                 &tdecl.body,
+                                Some(rref),
                             )?,
                             None => {
                                 invalids.insert(rref.clone());
@@ -308,7 +343,12 @@ impl TransitiveChecker<'_> {
                     Some(rdecl) => {
                         for field in rdecl.fields.iter() {
                             if let Some(ty) = &field.tp {
-                                self.collect_invalid_references(&mut invalids, module, ty)?;
+                                self.collect_invalid_references(
+                                    &mut invalids,
+                                    module,
+                                    ty,
+                                    Some(rref),
+                                )?;
                             }
                         }
                     }
@@ -321,25 +361,26 @@ impl TransitiveChecker<'_> {
                 invalids.insert(rref.clone());
             }
         };
-        let has_invalids = invalids.is_empty();
+        let no_invalids = invalids.is_empty();
         self.in_progress.remove(rref);
         self.invalid_refs.insert(rref.clone(), invalids);
-        Ok(has_invalids)
+        Ok(no_invalids)
     }
 
     fn collect_invalid_references(
         &mut self,
         refs: &mut FxHashSet<Ref>,
         module: &SmolStr,
         ty: &Type,
+        parent_ref: Option<&Ref>,
     ) -> Result<(), TransitiveCheckError> {
         match ty {
             Type::RemoteType(rt) => {
                 for arg in rt.arg_tys.iter() {
-                    self.collect_invalid_references(refs, module, arg)?;
+                    self.collect_invalid_references(refs, module, arg, parent_ref)?;
                 }
                 let rref = Ref::RidRef(rt.id.clone());
-                if !self.is_valid(&rref)? {
+                if !self.is_maybe_valid(&rref, parent_ref)? {
                     refs.insert(rref);
                 }
             }
@@ -348,20 +389,22 @@ impl TransitiveChecker<'_> {
             }
             Type::RecordType(rt) => {
                 let rref = Ref::RecRef(module.clone(), rt.name.clone());
-                if !self.is_valid(&rref)? {
+                if !self.is_maybe_valid(&rref, parent_ref)? {
                     refs.insert(rref);
                 }
             }
             Type::RefinedRecordType(rt) => {
                 let rref = Ref::RecRef(module.clone(), rt.rec_type.name.clone());
                 for (_, ty) in rt.fields.iter() {
-                    self.collect_invalid_references(refs, module, ty)?;
+                    self.collect_invalid_references(refs, module, ty, parent_ref)?;
                 }
-                if !self.is_valid(&rref)? {
+                if !self.is_maybe_valid(&rref, parent_ref)? {
                     refs.insert(rref);
                 }
             }
-            ty => ty.walk(&mut |ty| self.collect_invalid_references(refs, module, ty))?,
+            ty => {
+                ty.walk(&mut |ty| self.collect_invalid_references(refs, module, ty, parent_ref))?
+            }
         }
         Ok(())
     }